nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

[PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

Posted by Alexander Ivanov 1 year, 3 months ago

In some cases, the NBD server can be stopped before
nbd_blockdev_client_closed() is called, causing the nbd_server variable
to be nullified. This leads to a NULL pointer dereference when accessing
nbd_server.

Add a NULL check for nbd_server to the nbd_blockdev_client_closed()
function to prevent NULL pointer dereference.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 blockdev-nbd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 213012435f..fb1f30ae0d 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -52,6 +52,9 @@ int nbd_server_max_connections(void)
 static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
 {
     nbd_client_put(client);
+    if (nbd_server == NULL) {
+        return;
+    }
     assert(nbd_server->connections > 0);
     nbd_server->connections--;
     nbd_update_server_watch(nbd_server);
-- 
2.43.0

Re: [PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

Posted by Alexander Ivanov 1 year, 2 months ago

Ping?

On 6/7/24 17:00, Alexander Ivanov wrote:
>   static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>   {
>       nbd_client_put(client);
> +    if (nbd_server == NULL) {
> +        return;
> +    }
>       assert(nbd_server->connections > 0);
>       nbd_server->connections--;
>       nbd_update_server_watch(nbd_server);

-- 
Best regards,
Alexander Ivanov

Re: [PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

Posted by Alexander Ivanov 1 year, 3 months ago

There is a bug reproducer in the attachment.


On 6/7/24 17:00, Alexander Ivanov wrote:
> In some cases, the NBD server can be stopped before
> nbd_blockdev_client_closed() is called, causing the nbd_server variable
> to be nullified. This leads to a NULL pointer dereference when accessing
> nbd_server.
>
> Add a NULL check for nbd_server to the nbd_blockdev_client_closed()
> function to prevent NULL pointer dereference.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   blockdev-nbd.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index 213012435f..fb1f30ae0d 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -52,6 +52,9 @@ int nbd_server_max_connections(void)
>   static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>   {
>       nbd_client_put(client);
> +    if (nbd_server == NULL) {
> +        return;
> +    }
>       assert(nbd_server->connections > 0);
>       nbd_server->connections--;
>       nbd_update_server_watch(nbd_server);

-- 
Best regards,
Alexander Ivanov

Re: [PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

Posted by Eric Blake 1 year, 3 months ago

On Sat, Jun 08, 2024 at 11:36:59AM GMT, Alexander Ivanov wrote:
> There is a bug reproducer in the attachment.

Summarizing the reproducer, you are repeatedly calling QMP
nbd-server-start/nbd-server-stop on qemu as NBD server in one thread,
and repeatedly calling 'qemu-nbd -L' in another, to try and provoke
the race where the server is stopped while qemu-nbd -L is still trying
to grab information.

> 
> 
> On 6/7/24 17:00, Alexander Ivanov wrote:
> > In some cases, the NBD server can be stopped before
> > nbd_blockdev_client_closed() is called, causing the nbd_server variable
> > to be nullified. This leads to a NULL pointer dereference when accessing
> > nbd_server.

Am I correct that the NULL pointer deref was in qemu-nbd in your
reproducer, and not in qemu-kvm?

> > 
> > Add a NULL check for nbd_server to the nbd_blockdev_client_closed()
> > function to prevent NULL pointer dereference.
> > 
> > Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> > ---
> >   blockdev-nbd.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> > index 213012435f..fb1f30ae0d 100644
> > --- a/blockdev-nbd.c
> > +++ b/blockdev-nbd.c
> > @@ -52,6 +52,9 @@ int nbd_server_max_connections(void)
> >   static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
> >   {
> >       nbd_client_put(client);
> > +    if (nbd_server == NULL) {
> > +        return;
> > +    }
> >       assert(nbd_server->connections > 0);

While this does indeed avoid the NULL dereference right here, I still
want to understand why nbd_server is getting set to NULL while there
is still an active client, and make sure we don't have any other NULL
derefs.  I'll respond again once I've studied the code a bit more.

> >       nbd_server->connections--;
> >       nbd_update_server_watch(nbd_server);
> 
> -- 
> Best regards,
> Alexander Ivanov

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

Posted by Alexander Ivanov 1 year, 3 months ago

Hello Eric,

Do you have any ideas about the bug?

Thank you.

On 6/10/24 14:33, Eric Blake wrote:
> On Sat, Jun 08, 2024 at 11:36:59AM GMT, Alexander Ivanov wrote:
>> There is a bug reproducer in the attachment.
> Summarizing the reproducer, you are repeatedly calling QMP
> nbd-server-start/nbd-server-stop on qemu as NBD server in one thread,
> and repeatedly calling 'qemu-nbd -L' in another, to try and provoke
> the race where the server is stopped while qemu-nbd -L is still trying
> to grab information.
>
>>
>> On 6/7/24 17:00, Alexander Ivanov wrote:
>>> In some cases, the NBD server can be stopped before
>>> nbd_blockdev_client_closed() is called, causing the nbd_server variable
>>> to be nullified. This leads to a NULL pointer dereference when accessing
>>> nbd_server.
> Am I correct that the NULL pointer deref was in qemu-nbd in your
> reproducer, and not in qemu-kvm?
>
>>> Add a NULL check for nbd_server to the nbd_blockdev_client_closed()
>>> function to prevent NULL pointer dereference.
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>>    blockdev-nbd.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>>> index 213012435f..fb1f30ae0d 100644
>>> --- a/blockdev-nbd.c
>>> +++ b/blockdev-nbd.c
>>> @@ -52,6 +52,9 @@ int nbd_server_max_connections(void)
>>>    static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>>>    {
>>>        nbd_client_put(client);
>>> +    if (nbd_server == NULL) {
>>> +        return;
>>> +    }
>>>        assert(nbd_server->connections > 0);
> While this does indeed avoid the NULL dereference right here, I still
> want to understand why nbd_server is getting set to NULL while there
> is still an active client, and make sure we don't have any other NULL
> derefs.  I'll respond again once I've studied the code a bit more.
>
>>>        nbd_server->connections--;
>>>        nbd_update_server_watch(nbd_server);
>> -- 
>> Best regards,
>> Alexander Ivanov

-- 
Best regards,
Alexander Ivanov

Re: [PATCH] nbd: Prevent NULL pointer dereference in nbd_blockdev_client_closed()

Posted by Alexander Ivanov 1 year, 3 months ago


On 6/10/24 14:33, Eric Blake wrote:
> On Sat, Jun 08, 2024 at 11:36:59AM GMT, Alexander Ivanov wrote:
>> There is a bug reproducer in the attachment.
> Summarizing the reproducer, you are repeatedly calling QMP
> nbd-server-start/nbd-server-stop on qemu as NBD server in one thread,
> and repeatedly calling 'qemu-nbd -L' in another, to try and provoke
> the race where the server is stopped while qemu-nbd -L is still trying
> to grab information.
Yes, you are right.
>
>>
>> On 6/7/24 17:00, Alexander Ivanov wrote:
>>> In some cases, the NBD server can be stopped before
>>> nbd_blockdev_client_closed() is called, causing the nbd_server variable
>>> to be nullified. This leads to a NULL pointer dereference when accessing
>>> nbd_server.
> Am I correct that the NULL pointer deref was in qemu-nbd in your
> reproducer, and not in qemu-kvm?
No, it happened in qemu-kvm.
>
>>> Add a NULL check for nbd_server to the nbd_blockdev_client_closed()
>>> function to prevent NULL pointer dereference.
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>>    blockdev-nbd.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>>> index 213012435f..fb1f30ae0d 100644
>>> --- a/blockdev-nbd.c
>>> +++ b/blockdev-nbd.c
>>> @@ -52,6 +52,9 @@ int nbd_server_max_connections(void)
>>>    static void nbd_blockdev_client_closed(NBDClient *client, bool ignored)
>>>    {
>>>        nbd_client_put(client);
>>> +    if (nbd_server == NULL) {
>>> +        return;
>>> +    }
>>>        assert(nbd_server->connections > 0);
> While this does indeed avoid the NULL dereference right here, I still
> want to understand why nbd_server is getting set to NULL while there
> is still an active client, and make sure we don't have any other NULL
> derefs.  I'll respond again once I've studied the code a bit more.
I agree that the patch fixes only symptoms, but haven't succeeded with the
bug roots investigation, and decided to raise this issue with the community.

Also, later I reproduced another bug with the patch applied and with the
same reproducer. There was an

    assert(nbd_server->connections > 0);

violation in nbd_blockdev_client_closed(). It happens much less frequently,
however. Think the reasons are similar to the original bug.
>
>>>        nbd_server->connections--;
>>>        nbd_update_server_watch(nbd_server);
>> -- 
>> Best regards,
>> Alexander Ivanov

-- 
Best regards,
Alexander Ivanov