[PATCH v2 0/4] ses: prevent from out of bounds accesses

[PATCH v2 0/4] ses: prevent from out of bounds accesses

[Tomas Henzl]

First patch fixes a KASAN reported problem
Second patch fixes other possible places in ses_enclosure_data_process
where the max_desc_len might access memory out of bounds.
3/4 does the same for desc_ptr in ses_enclosure_data_process.
The last patch fixes another KASAN report in ses_intf_remove.

Changes:
v1: cc-ed stable@vger.kernel.org

Tomas Henzl (4):
  ses: fix slab-out-of-bounds reported by KASAN in ses_enclosure_data_process
  ses: fix possible addl_desc_ptr out-of-bounds accesses in ses_enclosure_data_process
  ses: fix possible desc_ptr out-of-bounds accesses in ses_enclosure_data_process
  ses: fix slab-out-of-bounds reported by KASAN in ses_intf_remove 

 drivers/scsi/ses.c | 58 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 17 deletions(-)

-- 
2.38.1

[PATCH v2 1/4] ses: fix slab-out-of-bounds reported by KASAN in ses_enclosure_data_process

[Tomas Henzl]

A fix for:
BUG: KASAN: slab-out-of-bounds in ses_enclosure_data_process+0x949/0xe30 [ses]
Read of size 1 at addr ffff88a1b043a451 by task systemd-udevd/3271
Checking after (and before in next loop) addl_desc_ptr[1] is sufficient,
we expect the size to be sanitized before first access to addl_desc_ptr[1].
Testing for one more byte shall protect partially the ses_process_descriptor.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/ses.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 0a1734f34587..1ee927ac8603 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -603,9 +603,11 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 			     /* these elements are optional */
 			     type_ptr[0] == ENCLOSURE_COMPONENT_SCSI_TARGET_PORT ||
 			     type_ptr[0] == ENCLOSURE_COMPONENT_SCSI_INITIATOR_PORT ||
-			     type_ptr[0] == ENCLOSURE_COMPONENT_CONTROLLER_ELECTRONICS))
+			     type_ptr[0] == ENCLOSURE_COMPONENT_CONTROLLER_ELECTRONICS)){
 				addl_desc_ptr += addl_desc_ptr[1] + 2;
-
+				if (addl_desc_ptr + 1 >= ses_dev->page10 + ses_dev->page10_len)
+					addl_desc_ptr = NULL;
+			}
 		}
 	}
 	kfree(buf);
-- 
2.38.1

[PATCH v2 2/4] ses: fix possible addl_desc_ptr out-of-bounds accesses in ses_enclosure_data_process

[Tomas Henzl]

Sanitize possible addl_desc_ptr out-of-bounds accesses
in ses_enclosure_data_process.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/ses.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 1ee927ac8603..896fd4f6e93d 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -433,8 +433,8 @@ int ses_match_host(struct enclosure_device *edev, void *data)
 }
 #endif  /*  0  */
 
-static void ses_process_descriptor(struct enclosure_component *ecomp,
-				   unsigned char *desc)
+static int ses_process_descriptor(struct enclosure_component *ecomp,
+				   unsigned char *desc, int max_desc_len)
 {
 	int eip = desc[0] & 0x10;
 	int invalid = desc[0] & 0x80;
@@ -445,22 +445,32 @@ static void ses_process_descriptor(struct enclosure_component *ecomp,
 	unsigned char *d;
 
 	if (invalid)
-		return;
+		return 0;
 
 	switch (proto) {
 	case SCSI_PROTOCOL_FCP:
 		if (eip) {
+			if (max_desc_len <= 7)
+				return 1;
 			d = desc + 4;
 			slot = d[3];
 		}
 		break;
 	case SCSI_PROTOCOL_SAS:
+
 		if (eip) {
+			if (max_desc_len <= 27)
+				return 1;
 			d = desc + 4;
 			slot = d[3];
 			d = desc + 8;
-		} else
+		} else {
+			if (max_desc_len <= 23)
+				return 1;
 			d = desc + 4;
+		}
+
+
 		/* only take the phy0 addr */
 		addr = (u64)d[12] << 56 |
 			(u64)d[13] << 48 |
@@ -477,6 +487,8 @@ static void ses_process_descriptor(struct enclosure_component *ecomp,
 	}
 	ecomp->slot = slot;
 	scomp->addr = addr;
+
+	return 0;
 }
 
 struct efd {
@@ -549,7 +561,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 		/* skip past overall descriptor */
 		desc_ptr += len + 4;
 	}
-	if (ses_dev->page10)
+	if (ses_dev->page10 && ses_dev->page10_len > 9)
 		addl_desc_ptr = ses_dev->page10 + 8;
 	type_ptr = ses_dev->page1_types;
 	components = 0;
@@ -557,6 +569,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 		for (j = 0; j < type_ptr[1]; j++) {
 			char *name = NULL;
 			struct enclosure_component *ecomp;
+			int max_desc_len;
 
 			if (desc_ptr) {
 				if (desc_ptr >= buf + page7_len) {
@@ -583,10 +596,14 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 					ecomp = &edev->component[components++];
 
 				if (!IS_ERR(ecomp)) {
-					if (addl_desc_ptr)
-						ses_process_descriptor(
-							ecomp,
-							addl_desc_ptr);
+					if (addl_desc_ptr) {
+						max_desc_len = ses_dev->page10_len -
+						    (addl_desc_ptr - ses_dev->page10);
+						if (ses_process_descriptor(ecomp,
+						    addl_desc_ptr,
+						    max_desc_len))
+							addl_desc_ptr = NULL;
+					}
 					if (create)
 						enclosure_component_register(
 							ecomp);
-- 
2.38.1

[PATCH v2 3/4] ses: fix possible desc_ptr out-of-bounds accesses in ses_enclosure_data_process

[Tomas Henzl]

Sanitize possible desc_ptr out-of-bounds accesses
in ses_enclosure_data_process.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/ses.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 896fd4f6e93d..dbfe12f63c98 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -572,15 +572,19 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 			int max_desc_len;
 
 			if (desc_ptr) {
-				if (desc_ptr >= buf + page7_len) {
+				if (desc_ptr + 3 >= buf + page7_len) {
 					desc_ptr = NULL;
 				} else {
 					len = (desc_ptr[2] << 8) + desc_ptr[3];
 					desc_ptr += 4;
-					/* Add trailing zero - pushes into
-					 * reserved space */
-					desc_ptr[len] = '\0';
-					name = desc_ptr;
+					if (desc_ptr + len > buf + page7_len)
+						desc_ptr = NULL;
+					else {
+						/* Add trailing zero - pushes into
+						 * reserved space */
+						desc_ptr[len] = '\0';
+						name = desc_ptr;
+					}
 				}
 			}
 			if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
-- 
2.38.1

[PATCH v2 4/4] ses: fix slab-out-of-bounds reported by KASAN in ses_intf_remove

[Tomas Henzl]

A fix for:
BUG: KASAN: slab-out-of-bounds in ses_intf_remove+0x23f/0x270 [ses]
Read of size 8 at addr ffff88a10d32e5d8 by task rmmod/12013
When edev->components is zero, accessing edev->component[0]
members is wrong.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/ses.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index dbfe12f63c98..7a9eb54e8808 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -850,7 +850,8 @@ static void ses_intf_remove_enclosure(struct scsi_device *sdev)
 	kfree(ses_dev->page2);
 	kfree(ses_dev);
 
-	kfree(edev->component[0].scratch);
+	if (edev->components)
+		kfree(edev->component[0].scratch);
 
 	put_device(&edev->edev);
 	enclosure_unregister(edev);
-- 
2.38.1

Re: [PATCH v2 0/4] ses: prevent from out of bounds accesses

[Martin K. Petersen]

Tomas,

> First patch fixes a KASAN reported problem Second patch fixes other
> possible places in ses_enclosure_data_process where the max_desc_len
> might access memory out of bounds.  3/4 does the same for desc_ptr in
> ses_enclosure_data_process.  The last patch fixes another KASAN report
> in ses_intf_remove.

Thanks for working on this! With your series applied, in combination
with a straggling patch from James, I can finally boot my SAS test setup
without any KASAN warnings.

Applied to 6.3/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 0/4] ses: prevent from out of bounds accesses

[Tomas Henzl]

On 2/21/23 23:59, Martin K. Petersen wrote:
> 
> Tomas,
> 
>> First patch fixes a KASAN reported problem Second patch fixes other
>> possible places in ses_enclosure_data_process where the max_desc_len
>> might access memory out of bounds.  3/4 does the same for desc_ptr in
>> ses_enclosure_data_process.  The last patch fixes another KASAN report
>> in ses_intf_remove.
> 
> Thanks for working on this! With your series applied, in combination
> with a straggling patch from James, I can finally boot my SAS test setup
> without any KASAN warnings.
I'm glad it worked for you and it could have been added since I've
noticed some previous approaches rejected.
> 
> Applied to 6.3/scsi-staging, thanks!
>