In kobject_get_path(), if kobj->name is changed between calls
get_kobj_path_length() and fill_kobj_path() and the length becomes
longer, then fill_kobj_path() will have an out-of-bounds bug.
The actual current problem occurs when the ixgbe probe.
In ixgbe_mii_bus_init(), if the length of netdev->dev.kobj.name
length becomes longer, out-of-bounds will occur.
cpu0 cpu1
ixgbe_probe
register_netdev(netdev)
netdev_register_kobject
device_add
kobject_uevent // Sending ADD events
systemd-udevd // rename netdev
dev_change_name
device_rename
kobject_rename
ixgbe_mii_bus_init |
mdiobus_register |
__mdiobus_register |
device_register |
device_add |
kobject_uevent |
kobject_get_path |
len = get_kobj_path_length // old name |
path = kzalloc(len, gfp_mask); |
kobj->name = name;
/* name length becomes
* longer
*/
fill_kobj_path /* kobj path length is
* longer than path,
* resulting in out of
* bounds when filling path
*/
This is the kasan report:
==================================================================
BUG: KASAN: slab-out-of-bounds in fill_kobj_path+0x50/0xc0
Write of size 7 at addr ff1100090573d1fd by task kworker/28:1/673
Workqueue: events work_for_cpu_fn
Call Trace:
<TASK>
dump_stack_lvl+0x34/0x48
print_address_description.constprop.0+0x86/0x1e7
print_report+0x36/0x4f
kasan_report+0xad/0x130
kasan_check_range+0x35/0x1c0
memcpy+0x39/0x60
fill_kobj_path+0x50/0xc0
kobject_get_path+0x5a/0xc0
kobject_uevent_env+0x140/0x460
device_add+0x5c7/0x910
__mdiobus_register+0x14e/0x490
ixgbe_probe.cold+0x441/0x574 [ixgbe]
local_pci_probe+0x78/0xc0
work_for_cpu_fn+0x26/0x40
process_one_work+0x3b6/0x6a0
worker_thread+0x368/0x520
kthread+0x165/0x1a0
ret_from_fork+0x1f/0x30
This reproducer triggers that bug:
while:
do
rmmod ixgbe
sleep 0.5
modprobe ixgbe
sleep 0.5
When calling fill_kobj_path() to fill path, if the name length of
kobj becomes longer, return failure and retry. This fixes the problem.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Wang Hai <[email protected]>
---
v1->v2: Return value type change and some formatting adjustments.
lib/kobject.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index a0b2dbfcfa23..3f97d903266a 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -112,7 +112,7 @@ static int get_kobj_path_length(struct kobject *kobj)
return length;
}
-static void fill_kobj_path(struct kobject *kobj, char *path, int length)
+static int fill_kobj_path(struct kobject *kobj, char *path, int length)
{
struct kobject *parent;
@@ -121,12 +121,16 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
int cur = strlen(kobject_name(parent));
/* back up enough to print this name with '/' */
length -= cur;
+ if (length <= 0)
+ return -EINVAL;
memcpy(path + length, kobject_name(parent), cur);
*(path + --length) = '/';
}
pr_debug("kobject: '%s' (%p): %s: path = '%s'\n", kobject_name(kobj),
kobj, __func__, path);
+
+ return 0;
}
/**
@@ -141,13 +145,17 @@ char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask)
char *path;
int len;
+retry:
len = get_kobj_path_length(kobj);
if (len == 0)
return NULL;
path = kzalloc(len, gfp_mask);
if (!path)
return NULL;
- fill_kobj_path(kobj, path, len);
+ if (fill_kobj_path(kobj, path, len)) {
+ kfree(path);
+ goto retry;
+ }
return path;
}
--
2.17.1
On Tue, Dec 20, 2022 at 09:21:43AM +0800, Wang Hai wrote:
> In kobject_get_path(), if kobj->name is changed between calls
> get_kobj_path_length() and fill_kobj_path() and the length becomes
> longer, then fill_kobj_path() will have an out-of-bounds bug.
>
> The actual current problem occurs when the ixgbe probe.
>
> In ixgbe_mii_bus_init(), if the length of netdev->dev.kobj.name
> length becomes longer, out-of-bounds will occur.
>
> cpu0 cpu1
> ixgbe_probe
> register_netdev(netdev)
> netdev_register_kobject
> device_add
> kobject_uevent // Sending ADD events
> systemd-udevd // rename netdev
> dev_change_name
> device_rename
> kobject_rename
> ixgbe_mii_bus_init |
> mdiobus_register |
> __mdiobus_register |
> device_register |
> device_add |
> kobject_uevent |
> kobject_get_path |
> len = get_kobj_path_length // old name |
> path = kzalloc(len, gfp_mask); |
> kobj->name = name;
> /* name length becomes
> * longer
> */
> fill_kobj_path /* kobj path length is
> * longer than path,
> * resulting in out of
> * bounds when filling path
> */
>
> This is the kasan report:
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in fill_kobj_path+0x50/0xc0
> Write of size 7 at addr ff1100090573d1fd by task kworker/28:1/673
>
> Workqueue: events work_for_cpu_fn
> Call Trace:
> <TASK>
> dump_stack_lvl+0x34/0x48
> print_address_description.constprop.0+0x86/0x1e7
> print_report+0x36/0x4f
> kasan_report+0xad/0x130
> kasan_check_range+0x35/0x1c0
> memcpy+0x39/0x60
> fill_kobj_path+0x50/0xc0
> kobject_get_path+0x5a/0xc0
> kobject_uevent_env+0x140/0x460
> device_add+0x5c7/0x910
> __mdiobus_register+0x14e/0x490
> ixgbe_probe.cold+0x441/0x574 [ixgbe]
> local_pci_probe+0x78/0xc0
> work_for_cpu_fn+0x26/0x40
> process_one_work+0x3b6/0x6a0
> worker_thread+0x368/0x520
> kthread+0x165/0x1a0
> ret_from_fork+0x1f/0x30
>
> This reproducer triggers that bug:
>
> while:
> do
> rmmod ixgbe
> sleep 0.5
> modprobe ixgbe
> sleep 0.5
>
> When calling fill_kobj_path() to fill path, if the name length of
> kobj becomes longer, return failure and retry. This fixes the problem.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Wang Hai <[email protected]>
> ---
> v1->v2: Return value type change and some formatting adjustments.
> lib/kobject.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index a0b2dbfcfa23..3f97d903266a 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -112,7 +112,7 @@ static int get_kobj_path_length(struct kobject *kobj)
> return length;
> }
>
> -static void fill_kobj_path(struct kobject *kobj, char *path, int length)
> +static int fill_kobj_path(struct kobject *kobj, char *path, int length)
> {
> struct kobject *parent;
>
> @@ -121,12 +121,16 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
> int cur = strlen(kobject_name(parent));
> /* back up enough to print this name with '/' */
> length -= cur;
> + if (length <= 0)
> + return -EINVAL;
> memcpy(path + length, kobject_name(parent), cur);
> *(path + --length) = '/';
> }
>
> pr_debug("kobject: '%s' (%p): %s: path = '%s'\n", kobject_name(kobj),
> kobj, __func__, path);
> +
> + return 0;
> }
>
> /**
> @@ -141,13 +145,17 @@ char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask)
> char *path;
> int len;
>
> +retry:
> len = get_kobj_path_length(kobj);
> if (len == 0)
> return NULL;
> path = kzalloc(len, gfp_mask);
> if (!path)
> return NULL;
> - fill_kobj_path(kobj, path, len);
> + if (fill_kobj_path(kobj, path, len)) {
> + kfree(path);
> + goto retry;
> + }
Thanks for the fix.
I wonder if there is no case we end up with infinite loop
(fill_kobj_path always returning error). Do You know?
>
> return path;
> }
> --
> 2.17.1
>
> _______________________________________________
> Intel-wired-lan mailing list
> [email protected]
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
在 2022/12/21 19:08, Michal Swiatkowski 写道:
> On Tue, Dec 20, 2022 at 09:21:43AM +0800, Wang Hai wrote:
>> In kobject_get_path(), if kobj->name is changed between calls
>> get_kobj_path_length() and fill_kobj_path() and the length becomes
>> longer, then fill_kobj_path() will have an out-of-bounds bug.
>>
>> The actual current problem occurs when the ixgbe probe.
>>
>> In ixgbe_mii_bus_init(), if the length of netdev->dev.kobj.name
>> length becomes longer, out-of-bounds will occur.
>>
>> cpu0 cpu1
>> ixgbe_probe
>> register_netdev(netdev)
>> netdev_register_kobject
>> device_add
>> kobject_uevent // Sending ADD events
>> systemd-udevd // rename netdev
>> dev_change_name
>> device_rename
>> kobject_rename
>> ixgbe_mii_bus_init |
>> mdiobus_register |
>> __mdiobus_register |
>> device_register |
>> device_add |
>> kobject_uevent |
>> kobject_get_path |
>> len = get_kobj_path_length // old name |
>> path = kzalloc(len, gfp_mask); |
>> kobj->name = name;
>> /* name length becomes
>> * longer
>> */
>> fill_kobj_path /* kobj path length is
>> * longer than path,
>> * resulting in out of
>> * bounds when filling path
>> */
>>
>> This is the kasan report:
>>
>> ==================================================================
>> BUG: KASAN: slab-out-of-bounds in fill_kobj_path+0x50/0xc0
>> Write of size 7 at addr ff1100090573d1fd by task kworker/28:1/673
>>
>> Workqueue: events work_for_cpu_fn
>> Call Trace:
>> <TASK>
>> dump_stack_lvl+0x34/0x48
>> print_address_description.constprop.0+0x86/0x1e7
>> print_report+0x36/0x4f
>> kasan_report+0xad/0x130
>> kasan_check_range+0x35/0x1c0
>> memcpy+0x39/0x60
>> fill_kobj_path+0x50/0xc0
>> kobject_get_path+0x5a/0xc0
>> kobject_uevent_env+0x140/0x460
>> device_add+0x5c7/0x910
>> __mdiobus_register+0x14e/0x490
>> ixgbe_probe.cold+0x441/0x574 [ixgbe]
>> local_pci_probe+0x78/0xc0
>> work_for_cpu_fn+0x26/0x40
>> process_one_work+0x3b6/0x6a0
>> worker_thread+0x368/0x520
>> kthread+0x165/0x1a0
>> ret_from_fork+0x1f/0x30
>>
>> This reproducer triggers that bug:
>>
>> while:
>> do
>> rmmod ixgbe
>> sleep 0.5
>> modprobe ixgbe
>> sleep 0.5
>>
>> When calling fill_kobj_path() to fill path, if the name length of
>> kobj becomes longer, return failure and retry. This fixes the problem.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Wang Hai <[email protected]>
>> ---
>> v1->v2: Return value type change and some formatting adjustments.
>> lib/kobject.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/kobject.c b/lib/kobject.c
>> index a0b2dbfcfa23..3f97d903266a 100644
>> --- a/lib/kobject.c
>> +++ b/lib/kobject.c
>> @@ -112,7 +112,7 @@ static int get_kobj_path_length(struct kobject *kobj)
>> return length;
>> }
>>
>> -static void fill_kobj_path(struct kobject *kobj, char *path, int length)
>> +static int fill_kobj_path(struct kobject *kobj, char *path, int length)
>> {
>> struct kobject *parent;
>>
>> @@ -121,12 +121,16 @@ static void fill_kobj_path(struct kobject *kobj, char *path, int length)
>> int cur = strlen(kobject_name(parent));
>> /* back up enough to print this name with '/' */
>> length -= cur;
>> + if (length <= 0)
>> + return -EINVAL;
>> memcpy(path + length, kobject_name(parent), cur);
>> *(path + --length) = '/';
>> }
>>
>> pr_debug("kobject: '%s' (%p): %s: path = '%s'\n", kobject_name(kobj),
>> kobj, __func__, path);
>> +
>> + return 0;
>> }
>>
>> /**
>> @@ -141,13 +145,17 @@ char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask)
>> char *path;
>> int len;
>>
>> +retry:
>> len = get_kobj_path_length(kobj);
>> if (len == 0)
>> return NULL;
>> path = kzalloc(len, gfp_mask);
>> if (!path)
>> return NULL;
>> - fill_kobj_path(kobj, path, len);
>> + if (fill_kobj_path(kobj, path, len)) {
>> + kfree(path);
>> + goto retry;
>> + }
> Thanks for the fix.
>
> I wonder if there is no case we end up with infinite loop
> (fill_kobj_path always returning error). Do You know?
It should only be possible to have an infinite loop if name or parent
keeps changing. The probability of this is extremely low.
If necessary, change it to only retry 3 times?
>>
>> return path;
>> }
>> --
>> 2.17.1
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> [email protected]
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> .
--
Wang Hai
在 2022/12/20 9:21, Wang Hai 写道:
> In kobject_get_path(), if kobj->name is changed between calls
> get_kobj_path_length() and fill_kobj_path() and the length becomes
> longer, then fill_kobj_path() will have an out-of-bounds bug.
>
> The actual current problem occurs when the ixgbe probe.
>
> In ixgbe_mii_bus_init(), if the length of netdev->dev.kobj.name
> length becomes longer, out-of-bounds will occur.
>
> cpu0 cpu1
> ixgbe_probe
> register_netdev(netdev)
> netdev_register_kobject
> device_add
> kobject_uevent // Sending ADD events
> systemd-udevd // rename netdev
> dev_change_name
> device_rename
> kobject_rename
> ixgbe_mii_bus_init |
> mdiobus_register |
> __mdiobus_register |
> device_register |
> device_add |
> kobject_uevent |
> kobject_get_path |
> len = get_kobj_path_length // old name |
> path = kzalloc(len, gfp_mask); |
> kobj->name = name;
> /* name length becomes
> * longer
> */
> fill_kobj_path /* kobj path length is
> * longer than path,
> * resulting in out of
> * bounds when filling path
> */
>
> This is the kasan report:
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in fill_kobj_path+0x50/0xc0
> Write of size 7 at addr ff1100090573d1fd by task kworker/28:1/673
>
> Workqueue: events work_for_cpu_fn
> Call Trace:
> <TASK>
> dump_stack_lvl+0x34/0x48
> print_address_description.constprop.0+0x86/0x1e7
> print_report+0x36/0x4f
> kasan_report+0xad/0x130
> kasan_check_range+0x35/0x1c0
> memcpy+0x39/0x60
> fill_kobj_path+0x50/0xc0
> kobject_get_path+0x5a/0xc0
> kobject_uevent_env+0x140/0x460
> device_add+0x5c7/0x910
> __mdiobus_register+0x14e/0x490
> ixgbe_probe.cold+0x441/0x574 [ixgbe]
> local_pci_probe+0x78/0xc0
> work_for_cpu_fn+0x26/0x40
> process_one_work+0x3b6/0x6a0
> worker_thread+0x368/0x520
> kthread+0x165/0x1a0
> ret_from_fork+0x1f/0x30
>
> This reproducer triggers that bug:
>
> while:
> do
> rmmod ixgbe
> sleep 0.5
> modprobe ixgbe
> sleep 0.5
>
> When calling fill_kobj_path() to fill path, if the name length of
> kobj becomes longer, return failure and retry. This fixes the problem.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Wang Hai <[email protected]>
> ---
Hi, greg k-h.
Sorry to bother you. Can this patch be merged into the mainline?
--
Wang Hai
在 2023/1/9 18:33, Greg KH 写道:
> On Mon, Jan 09, 2023 at 05:37:23PM +0800, Wang Hai wrote:
>> 在 2022/12/20 9:21, Wang Hai 写道:
>>> In kobject_get_path(), if kobj->name is changed between calls
>>> get_kobj_path_length() and fill_kobj_path() and the length becomes
>>> longer, then fill_kobj_path() will have an out-of-bounds bug.
>>>
>>> The actual current problem occurs when the ixgbe probe.
>>>
>>> In ixgbe_mii_bus_init(), if the length of netdev->dev.kobj.name
>>> length becomes longer, out-of-bounds will occur.
>>>
>>> cpu0 cpu1
>>> ixgbe_probe
>>> register_netdev(netdev)
>>> netdev_register_kobject
>>> device_add
>>> kobject_uevent // Sending ADD events
>>> systemd-udevd // rename netdev
>>> dev_change_name
>>> device_rename
>>> kobject_rename
>>> ixgbe_mii_bus_init |
>>> mdiobus_register |
>>> __mdiobus_register |
>>> device_register |
>>> device_add |
>>> kobject_uevent |
>>> kobject_get_path |
>>> len = get_kobj_path_length // old name |
>>> path = kzalloc(len, gfp_mask); |
>>> kobj->name = name;
>>> /* name length becomes
>>> * longer
>>> */
>>> fill_kobj_path /* kobj path length is
>>> * longer than path,
>>> * resulting in out of
>>> * bounds when filling path
>>> */
>>>
>>> This is the kasan report:
>>>
>>> ==================================================================
>>> BUG: KASAN: slab-out-of-bounds in fill_kobj_path+0x50/0xc0
>>> Write of size 7 at addr ff1100090573d1fd by task kworker/28:1/673
>>>
>>> Workqueue: events work_for_cpu_fn
>>> Call Trace:
>>> <TASK>
>>> dump_stack_lvl+0x34/0x48
>>> print_address_description.constprop.0+0x86/0x1e7
>>> print_report+0x36/0x4f
>>> kasan_report+0xad/0x130
>>> kasan_check_range+0x35/0x1c0
>>> memcpy+0x39/0x60
>>> fill_kobj_path+0x50/0xc0
>>> kobject_get_path+0x5a/0xc0
>>> kobject_uevent_env+0x140/0x460
>>> device_add+0x5c7/0x910
>>> __mdiobus_register+0x14e/0x490
>>> ixgbe_probe.cold+0x441/0x574 [ixgbe]
>>> local_pci_probe+0x78/0xc0
>>> work_for_cpu_fn+0x26/0x40
>>> process_one_work+0x3b6/0x6a0
>>> worker_thread+0x368/0x520
>>> kthread+0x165/0x1a0
>>> ret_from_fork+0x1f/0x30
>>>
>>> This reproducer triggers that bug:
>>>
>>> while:
>>> do
>>> rmmod ixgbe
>>> sleep 0.5
>>> modprobe ixgbe
>>> sleep 0.5
>>>
>>> When calling fill_kobj_path() to fill path, if the name length of
>>> kobj becomes longer, return failure and retry. This fixes the problem.
>>>
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Signed-off-by: Wang Hai <[email protected]>
>>> ---
>> Hi, greg k-h.
>> Sorry to bother you. Can this patch be merged into the mainline?
> It's in my "to review" queue that I am working on. As this is not
> anything that a normal user can trigger, it's not that high of a
> priority, right?
>
> thanks,
>
> greg k-h
> .
Thanks, I thought you had forgotten about it. I hope I'm not disturbing you.
--
Wang Hai
On Mon, Jan 09, 2023 at 05:37:23PM +0800, Wang Hai wrote:
>
> 在 2022/12/20 9:21, Wang Hai 写道:
> > In kobject_get_path(), if kobj->name is changed between calls
> > get_kobj_path_length() and fill_kobj_path() and the length becomes
> > longer, then fill_kobj_path() will have an out-of-bounds bug.
> >
> > The actual current problem occurs when the ixgbe probe.
> >
> > In ixgbe_mii_bus_init(), if the length of netdev->dev.kobj.name
> > length becomes longer, out-of-bounds will occur.
> >
> > cpu0 cpu1
> > ixgbe_probe
> > register_netdev(netdev)
> > netdev_register_kobject
> > device_add
> > kobject_uevent // Sending ADD events
> > systemd-udevd // rename netdev
> > dev_change_name
> > device_rename
> > kobject_rename
> > ixgbe_mii_bus_init |
> > mdiobus_register |
> > __mdiobus_register |
> > device_register |
> > device_add |
> > kobject_uevent |
> > kobject_get_path |
> > len = get_kobj_path_length // old name |
> > path = kzalloc(len, gfp_mask); |
> > kobj->name = name;
> > /* name length becomes
> > * longer
> > */
> > fill_kobj_path /* kobj path length is
> > * longer than path,
> > * resulting in out of
> > * bounds when filling path
> > */
> >
> > This is the kasan report:
> >
> > ==================================================================
> > BUG: KASAN: slab-out-of-bounds in fill_kobj_path+0x50/0xc0
> > Write of size 7 at addr ff1100090573d1fd by task kworker/28:1/673
> >
> > Workqueue: events work_for_cpu_fn
> > Call Trace:
> > <TASK>
> > dump_stack_lvl+0x34/0x48
> > print_address_description.constprop.0+0x86/0x1e7
> > print_report+0x36/0x4f
> > kasan_report+0xad/0x130
> > kasan_check_range+0x35/0x1c0
> > memcpy+0x39/0x60
> > fill_kobj_path+0x50/0xc0
> > kobject_get_path+0x5a/0xc0
> > kobject_uevent_env+0x140/0x460
> > device_add+0x5c7/0x910
> > __mdiobus_register+0x14e/0x490
> > ixgbe_probe.cold+0x441/0x574 [ixgbe]
> > local_pci_probe+0x78/0xc0
> > work_for_cpu_fn+0x26/0x40
> > process_one_work+0x3b6/0x6a0
> > worker_thread+0x368/0x520
> > kthread+0x165/0x1a0
> > ret_from_fork+0x1f/0x30
> >
> > This reproducer triggers that bug:
> >
> > while:
> > do
> > rmmod ixgbe
> > sleep 0.5
> > modprobe ixgbe
> > sleep 0.5
> >
> > When calling fill_kobj_path() to fill path, if the name length of
> > kobj becomes longer, return failure and retry. This fixes the problem.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Wang Hai <[email protected]>
> > ---
> Hi, greg k-h.
> Sorry to bother you. Can this patch be merged into the mainline?
It's in my "to review" queue that I am working on. As this is not
anything that a normal user can trigger, it's not that high of a
priority, right?
thanks,
greg k-h
On Mon, Jan 09, 2023 at 07:15:05PM +0800, Wang Hai wrote:
> Thanks, I thought you had forgotten about it. I hope I'm not disturbing you.
No bother at all, it's now added to my tree, thanks!
greg k-h