[v2] Fix race on srv->nclients_unauth and some other changes

[libvirt] [PATCH v2 14/14] remote: add locking around the critical section in remoteSASLFinish

Posted by Marc Hartmayer 7 years, 4 months ago

...as there is an access to priv->sasl the priv->lock is needed.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
---
 daemon/remote.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index b6fe6d8539ff..81d570b6e269 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -3389,6 +3389,9 @@ remoteSASLFinish(virNetServerPtr server,
     const char *identity;
     struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
     int ssf;
+    int rv = 0;
+
+    virMutexLock(&priv->lock);
 
     /* TLS or UNIX domain sockets trivially OK */
     if (!virNetServerClientIsSecure(client)) {
@@ -3398,15 +3401,15 @@ remoteSASLFinish(virNetServerPtr server,
         VIR_DEBUG("negotiated an SSF of %d", ssf);
         if (ssf < 56) { /* 56 is good for Kerberos */
             VIR_ERROR(_("negotiated SSF %d was not strong enough"), ssf);
-            return -2;
+            goto rejected;
         }
     }
 
     if (!(identity = virNetSASLSessionGetIdentity(priv->sasl)))
-        return -2;
+        goto rejected;
 
     if (!virNetSASLContextCheckIdentity(saslCtxt, identity))
-        return -2;
+        goto rejected;
 
     if (!(clnt_identity = virNetServerClientGetIdentity(client)))
         goto error;
@@ -3425,10 +3428,17 @@ remoteSASLFinish(virNetServerPtr server,
     virObjectUnref(priv->sasl);
     priv->sasl = NULL;
 
-    return 0;
+ cleanup:
+    virMutexUnlock(&priv->lock);
+    return rv;
 
  error:
-    return -1;
+    rv = -1;
+    goto cleanup;
+
+ rejected:
+    rv = -2;
+    goto cleanup;
 }
 
 /*
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 14/14] remote: add locking around the critical section in remoteSASLFinish

Posted by John Ferlan 7 years, 4 months ago


On 12/21/2017 09:29 AM, Marc Hartmayer wrote:
> ...as there is an access to priv->sasl the priv->lock is needed.
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
> ---
>  daemon/remote.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 

Both callers remoteDispatchAuthSaslStart and remoteDispatchAuthSaslStep
already have priv->lock taken (unless I'm missing something).


John

> diff --git a/daemon/remote.c b/daemon/remote.c
> index b6fe6d8539ff..81d570b6e269 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -3389,6 +3389,9 @@ remoteSASLFinish(virNetServerPtr server,
>      const char *identity;
>      struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
>      int ssf;
> +    int rv = 0;
> +
> +    virMutexLock(&priv->lock);
>  
>      /* TLS or UNIX domain sockets trivially OK */
>      if (!virNetServerClientIsSecure(client)) {
> @@ -3398,15 +3401,15 @@ remoteSASLFinish(virNetServerPtr server,
>          VIR_DEBUG("negotiated an SSF of %d", ssf);
>          if (ssf < 56) { /* 56 is good for Kerberos */
>              VIR_ERROR(_("negotiated SSF %d was not strong enough"), ssf);
> -            return -2;
> +            goto rejected;
>          }
>      }
>  
>      if (!(identity = virNetSASLSessionGetIdentity(priv->sasl)))
> -        return -2;
> +        goto rejected;
>  
>      if (!virNetSASLContextCheckIdentity(saslCtxt, identity))
> -        return -2;
> +        goto rejected;
>  
>      if (!(clnt_identity = virNetServerClientGetIdentity(client)))
>          goto error;
> @@ -3425,10 +3428,17 @@ remoteSASLFinish(virNetServerPtr server,
>      virObjectUnref(priv->sasl);
>      priv->sasl = NULL;
>  
> -    return 0;
> + cleanup:
> +    virMutexUnlock(&priv->lock);
> +    return rv;
>  
>   error:
> -    return -1;
> +    rv = -1;
> +    goto cleanup;
> +
> + rejected:
> +    rv = -2;
> +    goto cleanup;
>  }
>  
>  /*
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 14/14] remote: add locking around the critical section in remoteSASLFinish

Posted by Marc Hartmayer 7 years, 4 months ago

On Thu, Dec 21, 2017 at 07:20 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
> On 12/21/2017 09:29 AM, Marc Hartmayer wrote:
>> ...as there is an access to priv->sasl the priv->lock is needed.
>>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
>> ---
>>  daemon/remote.c | 20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
>
> Both callers remoteDispatchAuthSaslStart and remoteDispatchAuthSaslStep
> already have priv->lock taken (unless I'm missing something).


Ohhh, you’re right! Sry for that and thanks for checking!!

>
>
> John
>
>> diff --git a/daemon/remote.c b/daemon/remote.c
>> index b6fe6d8539ff..81d570b6e269 100644
>> --- a/daemon/remote.c
>> +++ b/daemon/remote.c
>> @@ -3389,6 +3389,9 @@ remoteSASLFinish(virNetServerPtr server,
>>      const char *identity;
>>      struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
>>      int ssf;
>> +    int rv = 0;
>> +
>> +    virMutexLock(&priv->lock);
>>
>>      /* TLS or UNIX domain sockets trivially OK */
>>      if (!virNetServerClientIsSecure(client)) {
>> @@ -3398,15 +3401,15 @@ remoteSASLFinish(virNetServerPtr server,
>>          VIR_DEBUG("negotiated an SSF of %d", ssf);
>>          if (ssf < 56) { /* 56 is good for Kerberos */
>>              VIR_ERROR(_("negotiated SSF %d was not strong enough"), ssf);
>> -            return -2;
>> +            goto rejected;
>>          }
>>      }
>>
>>      if (!(identity = virNetSASLSessionGetIdentity(priv->sasl)))
>> -        return -2;
>> +        goto rejected;
>>
>>      if (!virNetSASLContextCheckIdentity(saslCtxt, identity))
>> -        return -2;
>> +        goto rejected;
>>
>>      if (!(clnt_identity = virNetServerClientGetIdentity(client)))
>>          goto error;
>> @@ -3425,10 +3428,17 @@ remoteSASLFinish(virNetServerPtr server,
>>      virObjectUnref(priv->sasl);
>>      priv->sasl = NULL;
>>
>> -    return 0;
>> + cleanup:
>> +    virMutexUnlock(&priv->lock);
>> +    return rv;
>>
>>   error:
>> -    return -1;
>> +    rv = -1;
>> +    goto cleanup;
>> +
>> + rejected:
>> +    rv = -2;
>> +    goto cleanup;
>>  }
>>
>>  /*
>>
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list