linux-kernel - Re: [PATCH] rbd: avoid double free memory on error path in rbd_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06f51bab-42e1-975a-ad4f-6815c2063adb@redhat.com>
Date:   Tue, 7 Feb 2023 08:54:30 +0800
From:   Xiubo Li <xiubli@...hat.com>
To:     Петрова Наталия Михайловна <n.petrova@...tech.ru>,
        Ilya Dryomov <idryomov@...il.com>
Cc:     Dongsheng Yang <dongsheng.yang@...ystack.cn>,
        Jens Axboe <axboe@...nel.dk>,
        "ceph-devel@...r.kernel.org" <ceph-devel@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
        Alexey Khoroshilov <khoroshilov@...ras.ru>
Subject: Re: [PATCH] rbd: avoid double free memory on error path in
 rbd_dev_create()


On 06/02/2023 23:15, Петрова Наталия Михайловна wrote:
> Hi Ilya!
> Thanks for your response! I don't quite understand your idea and suggestion. The patch is designed to avoid double free memory. I explored the code again and suppose there is another situation for rbd_dev->rbd_client and rbd_dev->spec. Free memory of these pointers is possible only once in rbd_dev_free() function. In do_rbd_add() deallocation memory is only for rbd_opts: drivers/block/rbd.c 7157.

Hi Петрова,

If the rbd_dev_create() fails, for spec it will be freed in 
rbd_dev_create()->rbd_spec_put() first and then in do_rbd_add() it will 
call rbd_spec_put() again.

It won't trigger double free but this should generate a warning when the 
refcount underflow, because the refcount_dec_and_test() will warn and 
then return false when underflow happens.

The same for rbd_client.

Thanks,

- Xiubo

> Correct me if I'm wrong.
>
> Thanks,
> Natalia
>
> -----Original Message-----
> From: Ilya Dryomov <idryomov@...il.com>
> Sent: Monday, February 6, 2023 2:59 PM
> To: Петрова Наталия Михайловна <n.petrova@...tech.ru>
> Cc: Dongsheng Yang <dongsheng.yang@...ystack.cn>; Jens Axboe <axboe@...nel.dk>; ceph-devel@...r.kernel.org; linux-block@...r.kernel.org; linux-kernel@...r.kernel.org; lvc-project@...uxtesting.org; Alexey Khoroshilov <khoroshilov@...ras.ru>
> Subject: Re: [PATCH] rbd: avoid double free memory on error path in rbd_dev_create()
>
> On Fri, Feb 3, 2023 at 3:15 PM Natalia Petrova <n.petrova@...tech.ru> wrote:
>> If rbd_dev_create() fails after assignment 'opts' to 'rbd_dev->opts',
>> double free of 'rbd_options' happens:
>> one is in rbd_dev_free() and another one is in do_rbd_add().
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: 1643dfa4c2c8 ("rbd: introduce a per-device ordered workqueue")
>> Signed-off-by: Natalia Petrova <n.petrova@...tech.ru>
>> Signed-off-by: Alexey Khoroshilov <khoroshilov@...ras.ru>
>> ---
>>   drivers/block/rbd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index
>> 04453f4a319c..ab6bfc352cde 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -5357,7 +5357,6 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
>>          if (!rbd_dev)
>>                  return NULL;
>>
>> -       rbd_dev->opts = opts;
>>
>>          /* get an id and fill in device name */
>>          rbd_dev->dev_id = ida_simple_get(&rbd_dev_id_ida, 0, @@
>> -5372,6 +5371,7 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
>>          if (!rbd_dev->task_wq)
>>                  goto fail_dev_id;
>>
>> +       rbd_dev->opts = opts;
>>          /* we have a ref from do_rbd_add() */
>>          __module_get(THIS_MODULE);
>>
>> --
>> 2.34.1
>>
> Hi Natalia,
>
> It seems like a similar issue is affecting rbd_dev->rbd_client and rbd_dev->spec.  Unlike rbd_dev->opts, they are ref-counted and I'm guessing that the verification tool doesn't go that deep.
>
> I'd prefer all three to be addressed in the same change, since it's the same error path.  Would you be willing to look into that and post a new revision or should I treat just this patch as a bug report?
>
> Thanks,
>
>                  Ilya

-- 
Best Regards,

Xiubo Li (李秀波)

Email: xiubli@...hat.com/xiubli@....com
Slack: @Xiubo Li