From: Greg KH <gregkh@linuxfoundation.org> To: Wang Hai <wanghai38@huawei.com>, Alice Chao <alice.chao@mediatek.com> Cc: intel-wired-lan@lists.osuosl.org, anthony.l.nguyen@intel.com, jesse.brandeburg@intel.com, linux-kernel@vger.kernel.org, rafael@kernel.org Subject: Re: [Intel-wired-lan] [PATCH] kobject: Fix slab-out-of-bounds in fill_kobj_path() Date: Mon, 19 Dec 2022 15:54:40 +0100 [thread overview] Message-ID: <Y6B7MKZxjaoQlpgf@kroah.com> (raw) In-Reply-To: <20221219144103.34789-1-wanghai38@huawei.com> On Mon, Dec 19, 2022 at 10:41:03PM +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. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Wang Hai <wanghai38@huawei.com> > --- > lib/kobject.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) Nice find, and a much better description than was reported here: https://lore.kernel.org/all/20221216031320.2634-1-alice.chao@mediatek.com/ Alice, does this patch resolve your problem as well? > > diff --git a/lib/kobject.c b/lib/kobject.c > index a0b2dbfcfa23..d129f437b200 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 bool fill_kobj_path(struct kobject *kobj, char *path, int length) Just return an int and an error value, bool isn't a normal return value, right? > { > 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 false; Return -EINVAL or something? What if length ends up 0 here, is that going to be ok? > 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 true; return 0; > } > > /** > @@ -140,14 +144,20 @@ char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask) > { > char *path; > int len; > + bool ret; No need for this variable > > +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); > + ret = fill_kobj_path(kobj, path, len); > + if (!ret) { Make this one line: if (fill_kobj_path(kobj, path, len)) { But, you now have a loop, what guarantees that you can get out of it? thanks, greg k-h _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
WARNING: multiple messages have this Message-ID (diff)
From: Greg KH <gregkh@linuxfoundation.org> To: Wang Hai <wanghai38@huawei.com>, Alice Chao <alice.chao@mediatek.com> Cc: rafael@kernel.org, jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com, intel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] kobject: Fix slab-out-of-bounds in fill_kobj_path() Date: Mon, 19 Dec 2022 15:54:40 +0100 [thread overview] Message-ID: <Y6B7MKZxjaoQlpgf@kroah.com> (raw) In-Reply-To: <20221219144103.34789-1-wanghai38@huawei.com> On Mon, Dec 19, 2022 at 10:41:03PM +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. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Wang Hai <wanghai38@huawei.com> > --- > lib/kobject.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) Nice find, and a much better description than was reported here: https://lore.kernel.org/all/20221216031320.2634-1-alice.chao@mediatek.com/ Alice, does this patch resolve your problem as well? > > diff --git a/lib/kobject.c b/lib/kobject.c > index a0b2dbfcfa23..d129f437b200 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 bool fill_kobj_path(struct kobject *kobj, char *path, int length) Just return an int and an error value, bool isn't a normal return value, right? > { > 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 false; Return -EINVAL or something? What if length ends up 0 here, is that going to be ok? > 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 true; return 0; > } > > /** > @@ -140,14 +144,20 @@ char *kobject_get_path(struct kobject *kobj, gfp_t gfp_mask) > { > char *path; > int len; > + bool ret; No need for this variable > > +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); > + ret = fill_kobj_path(kobj, path, len); > + if (!ret) { Make this one line: if (fill_kobj_path(kobj, path, len)) { But, you now have a loop, what guarantees that you can get out of it? thanks, greg k-h
next prev parent reply other threads:[~2022-12-19 14:54 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-19 14:41 [PATCH] kobject: Fix slab-out-of-bounds in fill_kobj_path() Wang Hai 2022-12-19 14:41 ` [Intel-wired-lan] " Wang Hai 2022-12-19 14:54 ` Greg KH [this message] 2022-12-19 14:54 ` Greg KH 2022-12-19 15:27 ` wanghai (M) 2022-12-19 15:27 ` [Intel-wired-lan] " wanghai (M) 2022-12-19 15:39 ` Greg KH 2022-12-19 15:39 ` [Intel-wired-lan] " Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Y6B7MKZxjaoQlpgf@kroah.com \ --to=gregkh@linuxfoundation.org \ --cc=alice.chao@mediatek.com \ --cc=anthony.l.nguyen@intel.com \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jesse.brandeburg@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=rafael@kernel.org \ --cc=wanghai38@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.