[PATCH 0/2] scsi: ses: Fix out-of-bounds accesses

[PATCH 0/2] scsi: ses: Fix out-of-bounds accesses

[Ding Hui]

This series includes a few OOB fixes for ses driver

Ding Hui (1):
  scsi: ses: increase default init_alloc_size

Zhu Wei (1):
  scsi: ses: Fix slab-out-of-bounds in ses_get_power_status()

 drivers/scsi/ses.c | 55 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH 1/2] scsi: ses: Fix slab-out-of-bounds in ses_get_power_status()

[Ding Hui]

From: Zhu Wei <zhuwei@sangfor.com.cn>

A fix for:

BUG: KASAN: slab-out-of-bounds in ses_get_power_status+0x178/0x1a8 [ses]
Read of size 1 at addr ffffa5e3f9466a2b by task grep/102588
Call trace:
 ses_get_power_status+0x178/0x1a8 [ses]
 get_component_power_status+0x94/0x1f0 [enclosure]
 dev_attr_show+0x5c/0xc8
 sysfs_kf_seq_show+0x1b0/0x350
 kernfs_seq_show+0x10c/0x160
 seq_read+0x250/0xe28

If the page2 buffer is less than the number of elements indicated by page1,
that will cause desc_ptr to point to out-of-bounds.

The report occurs when we read sg power_status which iscsi target is a Dell
PowerVault MD3200i storage server.

Signed-off-by: Zhu Wei <zhuwei@sangfor.com.cn>
Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/scsi/ses.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index d7d0c35c58b8..2a404e51b6db 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -165,6 +165,8 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev,
 	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {
 		for (j = 0; j < type_ptr[1]; j++) {
 			desc_ptr += 4;
+			if (desc_ptr - ses_dev->page2 + 4 > ses_dev->page2_len)
+				break;
 			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
 			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 				continue;
@@ -196,6 +198,8 @@ static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 	for (i = 0; i < ses_dev->page1_num_types; i++, type_ptr += 4) {
 		for (j = 0; j < type_ptr[1]; j++) {
 			desc_ptr += 4;
+			if (desc_ptr - ses_dev->page2 + 4 > ses_dev->page2_len)
+				return NULL;
 			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
 			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 				continue;
-- 
2.17.1

[PATCH 2/2] scsi: ses: increase default init_alloc_size

[Ding Hui]

In 2020, I sent a patch [1] to address crash due to zero component count,
at that time, I did not dig why the ses driver got 0 but sg_ses tool got
the right component count. Now we have the answer.

In ses, when we prepare to receive a full diagnostic page{1,2,7,10},
we first detect the page length by a RECEIVE_DIAGNOSTIC request
with INIT_ALLOC_SIZE buffer len, we expect to get the correct page
length regardless of detecting request buffer len.

But for some storage device (e.g. vendor:DELL product:MD32xxi rev:0784),
its behavior is different. It replies the page length indicating that
it actually filled the buffer size in the response, rather that the
original page length it should be.

In this situation, the device reply hdr_buf[3]=28 since our detecting
request buffer is 32, we will allocate small buffer and save partial diag
page content, when we parse the page content, multiple types OOB reading
could be triggered. This is also the root cause of the OOB handled by
Zhu Wei in the previous patch.

The sg_ses directly invoke SG_IO ioctl with dxfer_len=65535, so it
always can report the right elements and descriptors.

To work well with this kind of devices, I increase the default
init_alloc_size to a relative large enough length for most devices,
and convert it to a module param in case some one need even larger
who warned by the log "Suspicious pageX len Y, try larger
init_alloc_size".

[1]: https://patchwork.kernel.org/patch/11938277

Cc: Zhu Wei <zhuwei@sangfor.com.cn>
Signed-off-by: Ding Hui <dinghui@sangfor.com.cn>
---
 drivers/scsi/ses.c | 51 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 2a404e51b6db..40069a4b51a8 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -35,6 +35,29 @@ struct ses_component {
 	u64 addr;
 };
 
+#define INIT_ALLOC_SIZE_DEF 4096
+#define INIT_ALLOC_SIZE_MIN 32
+#define INIT_ALLOC_SIZE_MAX 65535
+
+static uint init_alloc_size = INIT_ALLOC_SIZE_DEF;
+
+static int param_set_init_alloc_size(const char *val, const struct kernel_param *kp)
+{
+	uint size;
+	int ret = kstrtouint(val, 0, &size);
+
+	if (ret)
+		return ret;
+
+	init_alloc_size = clamp_t(uint, size, INIT_ALLOC_SIZE_MIN, INIT_ALLOC_SIZE_MAX);
+	return 0;
+}
+
+module_param_call(init_alloc_size, param_set_init_alloc_size, param_get_uint,
+		  &init_alloc_size, 0644);
+MODULE_PARM_DESC(init_alloc_size, "Buffer size for detecting diagnostic pages length. "
+		 "[Default=" __stringify(INIT_ALLOC_SIZE_DEF) "]");
+
 static bool ses_page2_supported(struct enclosure_device *edev)
 {
 	struct ses_device *ses_dev = edev->scratch;
@@ -525,8 +548,6 @@ static int ses_enclosure_find_by_addr(struct enclosure_device *edev,
 	return 0;
 }
 
-#define INIT_ALLOC_SIZE 32
-
 static void ses_enclosure_data_process(struct enclosure_device *edev,
 				       struct scsi_device *sdev,
 				       int create)
@@ -536,7 +557,8 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 	int i, j, page7_len, len, components;
 	struct ses_device *ses_dev = edev->scratch;
 	int types = ses_dev->page1_num_types;
-	unsigned char *hdr_buf = kzalloc(INIT_ALLOC_SIZE, GFP_KERNEL);
+	int hdr_buf_len = init_alloc_size;
+	unsigned char *hdr_buf = kzalloc(hdr_buf_len, GFP_KERNEL);
 
 	if (!hdr_buf)
 		goto simple_populate;
@@ -545,11 +567,14 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 	if (ses_dev->page10)
 		ses_recv_diag(sdev, 10, ses_dev->page10, ses_dev->page10_len);
 	/* Page 7 for the descriptors is optional */
-	result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
+	result = ses_recv_diag(sdev, 7, hdr_buf, hdr_buf_len);
 	if (result)
 		goto simple_populate;
 
 	page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	if (page7_len == hdr_buf_len)
+		sdev_printk(KERN_WARNING, sdev,
+			    "Suspicious page7 len %d, try larger init_alloc_size\n", page7_len);
 	/* add 1 for trailing '\0' we'll use */
 	buf = kzalloc(len + 1, GFP_KERNEL);
 	if (!buf)
@@ -678,6 +703,7 @@ static int ses_intf_add(struct device *cdev)
 	int num_enclosures;
 	struct enclosure_device *edev;
 	struct ses_component *scomp = NULL;
+	int hdr_buf_len = init_alloc_size;
 
 	if (!scsi_device_enclosure(sdev)) {
 		/* not an enclosure, but might be in one */
@@ -695,16 +721,19 @@ static int ses_intf_add(struct device *cdev)
 		sdev_printk(KERN_NOTICE, sdev, "Embedded Enclosure Device\n");
 
 	ses_dev = kzalloc(sizeof(*ses_dev), GFP_KERNEL);
-	hdr_buf = kzalloc(INIT_ALLOC_SIZE, GFP_KERNEL);
+	hdr_buf = kzalloc(hdr_buf_len, GFP_KERNEL);
 	if (!hdr_buf || !ses_dev)
 		goto err_init_free;
 
 	page = 1;
-	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
+	result = ses_recv_diag(sdev, page, hdr_buf, hdr_buf_len);
 	if (result)
 		goto recv_failed;
 
 	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	if (len == hdr_buf_len)
+		sdev_printk(KERN_WARNING, sdev,
+			    "Suspicious page1 len %d, try larger init_alloc_size\n", len);
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf)
 		goto err_free;
@@ -741,11 +770,14 @@ static int ses_intf_add(struct device *cdev)
 	buf = NULL;
 
 	page = 2;
-	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
+	result = ses_recv_diag(sdev, page, hdr_buf, hdr_buf_len);
 	if (result)
 		goto page2_not_supported;
 
 	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	if (len == hdr_buf_len)
+		sdev_printk(KERN_WARNING, sdev,
+			    "Suspicious page2 len %d, try larger init_alloc_size\n", len);
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf)
 		goto err_free;
@@ -761,10 +793,13 @@ static int ses_intf_add(struct device *cdev)
 	/* The additional information page --- allows us
 	 * to match up the devices */
 	page = 10;
-	result = ses_recv_diag(sdev, page, hdr_buf, INIT_ALLOC_SIZE);
+	result = ses_recv_diag(sdev, page, hdr_buf, hdr_buf_len);
 	if (!result) {
 
 		len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+		if (len == hdr_buf_len)
+			sdev_printk(KERN_WARNING, sdev,
+				    "Suspicious page10 len %d, try larger init_alloc_size\n", len);
 		buf = kzalloc(len, GFP_KERNEL);
 		if (!buf)
 			goto err_free;
-- 
2.17.1

Re: [PATCH 0/2] scsi: ses: Fix out-of-bounds accesses

[Ding Hui]

On 2023/11/30 22:28, Ding Hui wrote:
> This series includes a few OOB fixes for ses driver
> 
> Ding Hui (1):
>    scsi: ses: increase default init_alloc_size
> 
> Zhu Wei (1):
>    scsi: ses: Fix slab-out-of-bounds in ses_get_power_status()
> 
>   drivers/scsi/ses.c | 55 +++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 47 insertions(+), 8 deletions(-)
> 

Friendly ping.

-- 
Thanks,
- Ding Hui