diff mbox series

[1/1] scsi: Add length check to prevent invalid memory access

Message ID 20221216031320.2634-1-alice.chao@mediatek.com
State New
Headers
Series [1/1] scsi: Add length check to prevent invalid memory access |

Commit Message

Alice Chao Dec. 16, 2022, 3:13 a.m. UTC 
  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(+)

Comments

Greg KH Dec. 16, 2022, 6:47 a.m. UTC | #1 
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 mbox series

Patch

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) = '/';
 	}