arcmsr: buffer overflow in arcmsr_iop_message_xfer()

PATCH Superseded Dan Carpenter 5 participants 9 messages Sep 15, 2016 – Sep 27, 2016 1 message not in the archive SCSI

Share & tools

A message that is not in the archive (sent off-list or not yet received): CAFkTriJHaO0-O0AXKWSZW6bXT_2m+VqRdwshFsgeq1=Bjfn_OQ@mail.gmail.com

Dan Carpenter

author

2016-09-15 12:02 UTC #1 lore raw

PATCH 1/1 arcmsr: buffer overflow in arcmsr_iop_message_xfer()

We need to put an upper bound on "user_len" so the memcpy() doesn't overflow.

Reported-by Marco Grassi <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/arcmsr/arcmsr_hba.c +6 -1

index 7640498..73ddd45 100644
@@ -2388,7 +2388,8 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 	}
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
 		unsigned char *ver_addr;
-		int32_t user_len, cnt2end;
+		uint32_t user_len;
+		int32_t cnt2end;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
 		ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
 		if (!ver_addr) {

@@ -2397,6 +2398,10 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 		}
 		ptmpuserbuffer = ver_addr;
 		user_len = pcmdmessagefld->cmdmessage.Length;
+		if (user_len > ARCMSR_API_DATA_BUFLEN) {
+			retvalue = ARCMSR_MESSAGE_FAIL;
+			goto message_out;
+		}
 		memcpy(ptmpuserbuffer,
 			pcmdmessagefld->messagedatabuffer, user_len);
 		spin_lock_irqsave(&acb->wqbuffer_lock, flags);

Tomas Henzl

2016-09-15 13:19 UTC #2 lore raw

Quoted 21 lines · Dan Carpenter

We need to put an upper bound on "user_len" so the memcpy() doesn't overflow.

Reported-by Marco Grassi <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/arcmsr/arcmsr_hba.c +2 -1

index 7640498..73ddd45 100644
@@ -2388,7 +2388,8 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 	}
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
 		unsigned char *ver_addr;
-		int32_t user_len, cnt2end;
+		uint32_t user_len;
+		int32_t cnt2end;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
 		ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
 		if (!ver_addr) {

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, · line 2402

Quoted 5 lines

drivers/scsi/arcmsr/arcmsr_hba.c +2 -0

@@ -2397,6 +2398,10 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 		}
 		ptmpuserbuffer = ver_addr;
 		user_len = pcmdmessagefld->cmdmessage.Length;
+		if (user_len > ARCMSR_API_DATA_BUFLEN) {
+			retvalue = ARCMSR_MESSAGE_FAIL;

Hi,
I think that a 'kfree(ver_addr);' should be added here. With that you may add my reviewed-by.
Tomas

Quoted 9 lines

drivers/scsi/arcmsr/arcmsr_hba.c +2 -1
+			goto message_out;
+		}
 		memcpy(ptmpuserbuffer,
 			pcmdmessagefld->messagedatabuffer, user_len);
 		spin_lock_irqsave(&acb->wqbuffer_lock, flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2016-09-15 13:44 UTC #3 lore raw

Quoted 10 lines · Tomas Henzl

drivers/scsi/arcmsr/arcmsr_hba.c static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, · line 2402
Quoted 6 lines
drivers/scsi/arcmsr/arcmsr_hba.c +2 -0
@@ -2397,6 +2398,10 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 		}
 		ptmpuserbuffer = ver_addr;
 		user_len = pcmdmessagefld->cmdmessage.Length;
+		if (user_len > ARCMSR_API_DATA_BUFLEN) {
+			retvalue = ARCMSR_MESSAGE_FAIL;
Hi,
I think that a 'kfree(ver_addr);' should be added here. With that you may add my reviewed-by.

Oops... Thanks for catching that. I should have been more careful.

v2 on the way.

regards,
dan carpenter

Dan Carpenter

author

2016-09-15 13:45 UTC #4 lore raw

PATCH v2 1/1 arcmsr: buffer overflow in arcmsr_iop_message_xfer()

In reply to Tomas Henzl

We need to put an upper bound on "user_len" so the memcpy() doesn't overflow.

Reported-by Marco Grassi <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

Reviewed-by Tomas Henzl <[email protected]>

drivers/scsi/arcmsr/arcmsr_hba.c +7 -1

index 7640498..110eca9 100644
@@ -2388,7 +2388,8 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 	}
 	case ARCMSR_MESSAGE_WRITE_WQBUFFER: {
 		unsigned char *ver_addr;
-		int32_t user_len, cnt2end;
+		uint32_t user_len;
+		int32_t cnt2end;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
 		ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
 		if (!ver_addr) {

@@ -2397,6 +2398,11 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 		}
 		ptmpuserbuffer = ver_addr;
 		user_len = pcmdmessagefld->cmdmessage.Length;
+		if (user_len > ARCMSR_API_DATA_BUFLEN) {
+			retvalue = ARCMSR_MESSAGE_FAIL;
+			kfree(ver_addr);
+			goto message_out;
+		}
 		memcpy(ptmpuserbuffer,
 			pcmdmessagefld->messagedatabuffer, user_len);
 		spin_lock_irqsave(&acb->wqbuffer_lock, flags);

Martin K. Petersen

Applied

2016-09-15 13:59 UTC #5 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Dan" == Dan Carpenter <[email protected]> writes:

Dan> We need to put an upper bound on "user_len" so the memcpy() doesn't Dan> overflow.

Applied to 4.9/scsi-queue.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Borislav Petkov

2016-09-23 11:22 UTC #6 lore raw

Quoted 6 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Dan" == Dan Carpenter <[email protected]> writes:
Dan> We need to put an upper bound on "user_len" so the memcpy() doesn't Dan> overflow.
Applied to 4.9/scsi-queue.

Yap, Tomas said the kfree was missing on the error path but can we simplify this further by doing the user_len check first so that the kfree() is not even needed?

Patch ontop:

---
From: Borislav Petkov <[email protected]>
Date: Fri, 23 Sep 2016 13:04:51 +0200
Subject: [PATCH] scsi: arcmsr: Simplify user_len checking

Do the user_len check first and then the ver_addr allocation so that we can save us the kfree() on the error path when user_len is > ARCMSR_API_DATA_BUFLEN.

Signed-off-by Borislav Petkov <[email protected]>

Cc Marco Grassi <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc Tomas Henzl <[email protected]>

Cc Martin K. Petersen <[email protected]>

---
 drivers/scsi/arcmsr/arcmsr_hba.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/scsi/arcmsr/arcmsr_hba.c +8 -6

index 110eca9eaca0..3d53d636b17b 100644
@@ -2391,18 +2391,20 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb,
 		uint32_t user_len;
 		int32_t cnt2end;
 		uint8_t *pQbuffer, *ptmpuserbuffer;
-		ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
-		if (!ver_addr) {
+
+		user_len = pcmdmessagefld->cmdmessage.Length;
+		if (user_len > ARCMSR_API_DATA_BUFLEN) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
 			goto message_out;
 		}
-		ptmpuserbuffer = ver_addr;
-		user_len = pcmdmessagefld->cmdmessage.Length;
-		if (user_len > ARCMSR_API_DATA_BUFLEN) {
+
+		ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC);
+		if (!ver_addr) {
 			retvalue = ARCMSR_MESSAGE_FAIL;
-			kfree(ver_addr);
 			goto message_out;
 		}
+		ptmpuserbuffer = ver_addr;
+
 		memcpy(ptmpuserbuffer,
 			pcmdmessagefld->messagedatabuffer, user_len);
 		spin_lock_irqsave(&acb->wqbuffer_lock, flags);

Signature · 7 lines

1.8.5.2

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Johannes Thumshirn

Reviewed-by

2016-09-26 11:58 UTC #7 lore raw

Quoted 23 lines · Borislav Petkov

Quoted 6 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Dan" == Dan Carpenter <[email protected]> writes:
Dan> We need to put an upper bound on "user_len" so the memcpy() doesn't Dan> overflow.
Applied to 4.9/scsi-queue.
Yap, Tomas said the kfree was missing on the error path but can we simplify this further by doing the user_len check first so that the kfree() is not even needed?
Patch ontop:
---
From: Borislav Petkov <[email protected]>
Date: Fri, 23 Sep 2016 13:04:51 +0200
Subject: [PATCH] scsi: arcmsr: Simplify user_len checking
Do the user_len check first and then the ver_addr allocation so that we can save us the kfree() on the error path when user_len is > ARCMSR_API_DATA_BUFLEN.

Quoted 6 lines

Signed-off-by Borislav Petkov <[email protected]>
Cc Marco Grassi <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Tomas Henzl <[email protected]>
Cc Martin K. Petersen <[email protected]>
---

Looks good to me,

Reviewed-by Johannes Thumshirn <[email protected]>

Signature · 6 lines

Johannes Thumshirn                                          Storage
[email protected]                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Tomas Henzl

Reviewed-by

2016-09-26 13:29 UTC #8 lore raw

In reply to Borislav Petkov

Quoted 21 lines · Borislav Petkov

Quoted 5 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Dan" == Dan Carpenter <[email protected]> writes:
Dan> We need to put an upper bound on "user_len" so the memcpy() doesn't Dan> overflow.
Applied to 4.9/scsi-queue.
Yap, Tomas said the kfree was missing on the error path but can we simplify this further by doing the user_len check first so that the kfree() is not even needed?
Patch ontop:
---
From: Borislav Petkov <[email protected]>
Date: Fri, 23 Sep 2016 13:04:51 +0200
Subject: [PATCH] scsi: arcmsr: Simplify user_len checking
Do the user_len check first and then the ver_addr allocation so that we can save us the kfree() on the error path when user_len is > ARCMSR_API_DATA_BUFLEN.

Quoted 5 lines

Signed-off-by Borislav Petkov <[email protected]>
Cc Marco Grassi <[email protected]>
Cc Dan Carpenter <[email protected]>
Cc Tomas Henzl <[email protected]>
Cc Martin K. Petersen <[email protected]>

Looks good,

Reviewed-by Tomas Henzl <[email protected]>

Martin K. Petersen

Applied

2016-09-27 01:09 UTC #9 lore raw

In reply to Borislav Petkov

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Borislav" == Borislav Petkov <[email protected]> writes:

Borislav> Yap, Tomas said the kfree was missing on the error path but Borislav> can we simplify this further by doing the user_len check first Borislav> so that the kfree() is not even needed?

Applied to 4.9/scsi-queue.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering