* [PATCH] bcachefs: fix memleak in bch2_split_devs
@ 2024-01-07 7:05 Su Yue
2024-01-08 14:56 ` Brian Foster
0 siblings, 1 reply; 3+ messages in thread
From: Su Yue @ 2024-01-07 7:05 UTC (permalink / raw)
To: linux-bcachefs; +Cc: l, Su Yue
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;
+
if (!dev_name)
return -ENOMEM;
@@ -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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] bcachefs: fix memleak in bch2_split_devs
2024-01-07 7:05 [PATCH] bcachefs: fix memleak in bch2_split_devs Su Yue
@ 2024-01-08 14:56 ` Brian Foster
2024-01-08 15:05 ` Su Yue
0 siblings, 1 reply; 3+ messages in thread
From: Brian Foster @ 2024-01-08 14:56 UTC (permalink / raw)
To: Su Yue; +Cc: linux-bcachefs, l
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:
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bcachefs: fix memleak in bch2_split_devs
2024-01-08 14:56 ` Brian Foster
@ 2024-01-08 15:05 ` Su Yue
0 siblings, 0 replies; 3+ messages in thread
From: Su Yue @ 2024-01-08 15:05 UTC (permalink / raw)
To: Brian Foster; +Cc: Su Yue, linux-bcachefs, l
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
>>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-08 15:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-07 7:05 [PATCH] bcachefs: fix memleak in bch2_split_devs Su Yue
2024-01-08 14:56 ` Brian Foster
2024-01-08 15:05 ` Su Yue
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).