All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alice Chao <alice.chao@mediatek.com>
To: <gregkh@linuxfoundation.org>, <rafael@kernel.org>,
	<matthias.bgg@gmail.com>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>
Cc: <stanley.chu@mediatek.com>, <peter.wang@mediatek.com>,
	<chun-hung.wu@mediatek.com>, <alice.chao@mediatek.com>,
	<powen.kao@mediatek.com>, <naomi.chu@mediatek.com>,
	<cc.chou@mediatek.com>, <chaotian.jing@mediatek.com>,
	<jiajie.hao@mediatek.com>, <qilin.tan@mediatek.com>,
	<lin.gui@mediatek.com>, <yanxu.wei@mediatek.com>,
	<tun-yu.yu@mediatek.com>, <eddie.huang@mediatek.com>,
	<wsd_upstream@mediatek.com>
Subject: [PATCH 1/1] scsi: Add length check to prevent invalid memory access
Date: Fri, 16 Dec 2022 11:13:22 +0800	[thread overview]
Message-ID: <20221216031320.2634-1-alice.chao@mediatek.com> (raw)

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(+)

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


WARNING: multiple messages have this Message-ID (diff)
From: Alice Chao <alice.chao@mediatek.com>
To: <gregkh@linuxfoundation.org>, <rafael@kernel.org>,
	<matthias.bgg@gmail.com>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>
Cc: <stanley.chu@mediatek.com>, <peter.wang@mediatek.com>,
	<chun-hung.wu@mediatek.com>, <alice.chao@mediatek.com>,
	<powen.kao@mediatek.com>, <naomi.chu@mediatek.com>,
	<cc.chou@mediatek.com>, <chaotian.jing@mediatek.com>,
	<jiajie.hao@mediatek.com>, <qilin.tan@mediatek.com>,
	<lin.gui@mediatek.com>, <yanxu.wei@mediatek.com>,
	<tun-yu.yu@mediatek.com>, <eddie.huang@mediatek.com>,
	<wsd_upstream@mediatek.com>
Subject: [PATCH 1/1] scsi: Add length check to prevent invalid memory access
Date: Fri, 16 Dec 2022 11:13:22 +0800	[thread overview]
Message-ID: <20221216031320.2634-1-alice.chao@mediatek.com> (raw)

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(+)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

             reply	other threads:[~2022-12-16  3:16 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-16  3:13 Alice Chao [this message]
2022-12-16  3:13 ` [PATCH 1/1] scsi: Add length check to prevent invalid memory access Alice Chao
2022-12-16  6:47 ` Greg KH
2022-12-16  6:47   ` 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=20221216031320.2634-1-alice.chao@mediatek.com \
    --to=alice.chao@mediatek.com \
    --cc=cc.chou@mediatek.com \
    --cc=chaotian.jing@mediatek.com \
    --cc=chun-hung.wu@mediatek.com \
    --cc=eddie.huang@mediatek.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jiajie.hao@mediatek.com \
    --cc=lin.gui@mediatek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=naomi.chu@mediatek.com \
    --cc=peter.wang@mediatek.com \
    --cc=powen.kao@mediatek.com \
    --cc=qilin.tan@mediatek.com \
    --cc=rafael@kernel.org \
    --cc=stanley.chu@mediatek.com \
    --cc=tun-yu.yu@mediatek.com \
    --cc=wsd_upstream@mediatek.com \
    --cc=yanxu.wei@mediatek.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: link
Be 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.