Messages in this thread | | | Date | Fri, 16 Dec 2022 07:47:56 +0100 | From | Greg KH <> | Subject | Re: [PATCH 1/1] scsi: Add length check to prevent invalid memory access |
| |
On Fri, Dec 16, 2022 at 11:13:22AM +0800, Alice Chao wrote: > Device reset thread uses kobject_uevent_env() to get kobj.parent(kobj.p) > , and it races with device init thread which calls device_add() to add > kobj.parent before kobject_uevent_env().
Note, "scsi" in the subject line is wrong, this should be "kobject", right?
> > Device init call: Device reset call: > scsi_probe_and_add_lun() scsi_evt_thread()
Shouldn't the scsi layer be protecting these from happening at the same time? Or some other bus lock?
> scsi_add_lun() scsi_evt_emit() > scsi_sysfs_add_sdev() kobject_uevent_env() //get kobj.parent > scsi_target_add() kobject_get_path() > len = get_kobj_path_length () > //len=1 because parent hasn't created yet
Wait, how can the parent be gone if it has not been added yet? A parent should always be present, a device can not be there if it did not have a parent already.
> device_add() // add kobj.parent > kobject_uevent_env() > kobject_get_path() path = kzalloc() > fill_kobj_path() fill_kobj_path() > // --length; length -= cur is a negative value > memcpy(path + length, kobject_name(parent), cur); > // slab OOB! > > Above backtrace describes the problem, device reset thread will get wrong > kobj.parent when device init thread didn't add kobj/parent yet. When this > racing happened, it triggers the a KASAN dump on the final iteration: > > BUG: KASAN: slab-out-of-bounds in kobject_get_path+0xf8/0x1b8 > Write of size 11 at addr ffffff80d6bb94f5 by task kworker/3:1/58 > <snip> > > Call trace: > __kasan_report+0x124/0x1c8 > kasan_report+0x54/0x84 > kasan_check_range+0x200/0x208 > memcpy+0xb8/0xf0 > kobject_get_path+0xf8/0x1b8 > kobject_uevent_env+0x228/0xa88 > scsi_evt_thread+0x2d0/0x5b0 > process_one_work+0x570/0xf94 > worker_thread+0x7cc/0xf80 > kthread+0x2c4/0x388 > > These two jobs are scheduled asynchronously, we can't guaranteed that > kobj.parent will be created in device init thread before device reset > thread calls kobject_get_path().
Again, a parent should always be there, how is that happening?
> To prevent length -= cur from being a negative value, we add length > check in fill_kobj_path() to prevent invalid memory access. > > Signed-off-by: Alice Chao <alice.chao@mediatek.com> > --- > lib/kobject.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/kobject.c b/lib/kobject.c > index af1f5f2954d4..3cccb8e88d4e 100644 > --- a/lib/kobject.c > +++ b/lib/kobject.c > @@ -121,6 +121,10 @@ 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) > + break;
Shouldn't you just check for cur == 0 here instead?
But again, how can we have a parent with no name?
thanks,
greg k-h
| |