Re: [PATCH] bcachefs: fix memleak in bch2_split_devs

[Su Yue <l@damenly.org>]

On Mon 08 Jan 2024 at 09:56, Brian Foster <bfoster@redhat.com> 
wrote:

> On Sun, Jan 07, 2024 at 03:05:59PM +0800, Su Yue wrote:
>> The pointer dev_name can be modified by strseq(),
>> then causes the memleak:
>>
>> unreferenced object 0xffff9d08a2916c80 (size 32):
>>   comm "mount.bcachefs", pid 9090, jiffies 4295856224 (age 
>>   17.564s)
>>   hex dump (first 32 bytes):
>>     2f 64 65 76 2f 6d 61 70 70 65 72 2f 74 65 73 74 
>>     /dev/mapper/test
>>     2d 30 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>>     -0..............
>>   backtrace:
>>     [<00000000c5d3be7d>] __kmem_cache_alloc_node+0x1f3/0x2c0
>>     [<0000000052215d26>] __kmalloc_node_track_caller+0x51/0x150
>>     [<0000000069fea956>] kstrdup+0x32/0x60
>>     [<000000000877fcf1>] bch2_split_devs+0x3f/0x150 [bcachefs]
>>     [<000000007ee93204>] bch2_mount+0xcb/0x640 [bcachefs]
>>     [<000000002dd1e04b>] legacy_get_tree+0x30/0x60
>>     [<000000006afc31d3>] vfs_get_tree+0x28/0xf0
>>     [<000000007b0c538e>] path_mount+0x475/0xb60
>>     [<0000000092de5882>] __x64_sys_mount+0x105/0x140
>>     [<0000000054fc05d8>] do_syscall_64+0x42/0xf0
>>     [<00000000df584910>] 
>>     entry_SYSCALL_64_after_hwframe+0x6e/0x76
>>
>> Fix it by copying pointer dev_name at beginning then freeing 
>> the copied
>> pointer at end.
>>
>> Signed-off-by: Su Yue <glass.su@suse.com>
>> ---
>>  fs/bcachefs/util.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/bcachefs/util.c b/fs/bcachefs/util.c
>> index c2ef7cddaa4f..789953c14f55 100644
>> --- a/fs/bcachefs/util.c
>> +++ b/fs/bcachefs/util.c
>> @@ -1186,7 +1186,9 @@ int bch2_split_devs(const char 
>> *_dev_name, darray_str *ret)
>>  {
>>  	darray_init(ret);
>>
>> -	char *dev_name = kstrdup(_dev_name, GFP_KERNEL), *s = 
>> dev_name;
>> +	char *dev_name = kstrdup(_dev_name, GFP_KERNEL);
>> +	char *s = dev_name, *orig = dev_name;
>> +
>
> Thanks for the patch. I see this is just moving the assignment 
> of *s,
> but there's no need for that assignment in this code, right? If 
> not, I
> think this would look a little bit cleaner if it fixed that up 
> as well.
> I.e., something like:
>

Right. v2 sent ;)

--
Su
> 	char *s, *dev_name, *orig;
>
> 	dev_name = orig = kstrdup(_dev_name, GFP_KERNEL);
>>  	if (!dev_name)
>>  		return -ENOMEM;
>>
>
> ... and then otherwise the rest LGTM.
>
> Brian
>
>> @@ -1201,10 +1203,10 @@ int bch2_split_devs(const char 
>> *_dev_name, darray_str *ret)
>>  		}
>>  	}
>>
>> -	kfree(dev_name);
>> +	kfree(orig);
>>  	return 0;
>>  err:
>>  	bch2_darray_str_exit(ret);
>> -	kfree(dev_name);
>> +	kfree(orig);
>>  	return -ENOMEM;
>>  }
>> --
>> 2.43.0
>>
>>