Re: [PATCH V2 7/9] btrfs: fix null pointer dereference in clone_fs_devices when name is null

[Anand Jain <anand.jain@oracle.com>]

On 07/07/2014 12:22, Miao Xie wrote:
> On Mon, 7 Jul 2014 12:04:09 +0800, Anand Jain wrote:
>>> when one of the device path is missing btrfs_device name is null. So this
>>> patch will check for that.
>>>
>>> stack:
>>> BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
>>> IP: [<ffffffff812e18c0>] strlen+0x0/0x30
>>> [<ffffffffa01cd92a>] ? clone_fs_devices+0xaa/0x160 [btrfs]
>>> [<ffffffffa01cdcf7>] btrfs_init_new_device+0x317/0xca0 [btrfs]
>>> [<ffffffff81155bca>] ? __kmalloc_track_caller+0x15a/0x1a0
>>> [<ffffffffa01d6473>] btrfs_ioctl+0xaa3/0x2860 [btrfs]
>>> [<ffffffff81132a6c>] ? handle_mm_fault+0x48c/0x9c0
>>> [<ffffffff81192a61>] ? __blkdev_put+0x171/0x180
>>> [<ffffffff817a784c>] ? __do_page_fault+0x4ac/0x590
>>> [<ffffffff81193426>] ? blkdev_put+0x106/0x110
>>> [<ffffffff81179175>] ? mntput+0x35/0x40
>>> [<ffffffff8116d4b0>] do_vfs_ioctl+0x460/0x4a0
>>> [<ffffffff8115c72e>] ? ____fput+0xe/0x10
>>> [<ffffffff81068033>] ? task_work_run+0xb3/0xd0
>>> [<ffffffff8116d547>] SyS_ioctl+0x57/0x90
>>> [<ffffffff817a793e>] ? do_page_fault+0xe/0x10
>>> [<ffffffff817abe52>] system_call_fastpath+0x16/0x1b
>>>
>>> reproducer:
>>> mkfs.btrfs -draid1 -mraid1 /dev/sdg1 /dev/sdg2
>>> btrfstune -S 1 /dev/sdg1
>>> modprobe -r btrfs && modprobe btrfs
>>> mount -o degraded /dev/sdg1 /btrfs
>>> btrfs dev add /dev/sdg3 /btrfs
>>>
>>> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
>>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>>> ---
>>> Changelog v1->v2:
>>> - Fix the problem that we forgot to set the missing flag for the cloned device
>>> ---
>>>    fs/btrfs/volumes.c | 25 ++++++++++++++++---------
>>>    1 file changed, 16 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 1891541..4731bd6 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -598,16 +598,23 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
>>>            if (IS_ERR(device))
>>>                goto error;
>>>
>>> -        /*
>>> -         * This is ok to do without rcu read locked because we hold the
>>> -         * uuid mutex so nothing we touch in here is going to disappear.
>>> -         */
>>> -        name = rcu_string_strdup(orig_dev->name->str, GFP_NOFS);
>>> -        if (!name) {
>>> -            kfree(device);
>>> -            goto error;
>>> +        if (orig_dev->missing) {
>>> +            device->missing = 1;
>>> +            fs_devices->missing_devices++;
>>
>>   as mentioned in some places we just check name (for missing device)
>>   and  don't set the missing flag so it better to ..
>>
>>   if (orig_dev->missing || !orig_dev->name) {
>>              device->missing = 1;
>>              fs_devices->missing_devices++;
>
> I don't think we need check name pointer here because only missing device
> doesn't have its own name. Or there is something wrong in the code, so
> I add assert in else branch. Am I right?

At few critical code, the below and I guess in the chunk/strips
function as well, we don't make use of missing flag, but rather
->name.

-----
btrfsic_process_superblock
::
                 if (!device->bdev || !device->name)
                         continue;
-----

  But here without !orig_dev->name check, is also good enough.

Thanks, Anand


>>> +        } else {
>>> +            ASSERT(orig_dev->name);
>>> +            /*
>>> +             * This is ok to do without rcu read locked because
>>> +             * we hold the uuid mutex so nothing we touch in here
>>> +             * is going to disappear.
>>> +             */
>>> +            name = rcu_string_strdup(orig_dev->name->str, GFP_NOFS);
>>> +            if (!name) {
>>> +                kfree(device);
>>> +                goto error;
>>> +            }
>>> +            rcu_assign_pointer(device->name, name);
>>>            }
>>> -        rcu_assign_pointer(device->name, name);
>>>
>>>            list_add(&device->dev_list, &fs_devices->devices);
>>>            device->fs_devices = fs_devices;
>>>
>>
>> Thanks, Anand
>> .
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>