| Message ID | 20221216031320.2634-1-alice.chao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] scsi: Add length check to prevent invalid memory access | expand |
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
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; + memcpy(path + length, kobject_name(parent), cur); *(path + --length) = '/'; }
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(). Device init call: Device reset call: scsi_probe_and_add_lun() scsi_evt_thread() 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 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(). 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(+)