hpsa: fix an sprintf() overflow in the reset handler

PATCH In review Dan Carpenter 4 participants 6 messages Jun 4, 2015 – Jun 24, 2015

Subsystems (3)

SCSI Kernel janitors Hewlett-packard smart array RAID driver (hpsa)

lore

Share & tools

lore

Dan Carpenter

author

2015-06-04 14:47 UTC #0 lore raw

PATCH 1/1 hpsa: fix an sprintf() overflow in the reset handler

The string "cmd %d RESET FAILED, new lockup detected" is not quite large enough so the sprintf() will overflow. I have increased the size of the buffer and also changed the sprintf calls to snprintf.

Fixes 73153fe533bc ('hpsa: use block layer tag for command allocation')

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

drivers/scsi/hpsa.c +9 -6

index 1dafeb4..cab4e98 100644
@@ -5104,7 +5104,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	int rc;
 	struct ctlr_info *h;
 	struct hpsa_scsi_dev_t *dev;
-	char msg[40];
+	char msg[48];
 
 	/* find the controller to which the command to be aborted was sent */
 	h = sdev_to_hba(scsicmd->device);

@@ -5122,16 +5122,18 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 
 	/* if controller locked up, we can guarantee command won't complete */
 	if (lockup_detected(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
 
 	/* this reset request might be the result of a lockup; check */
 	if (detect_controller_lockup(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, new lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, new lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}

@@ -5145,7 +5147,8 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	/* send a reset to the SCSI LUN which the command was sent to */
 	rc = hpsa_do_reset(h, dev, dev->scsi3addr, HPSA_RESET_TYPE_LUN,
 			   DEFAULT_REPLY_QUEUE);
-	sprintf(msg, "reset %s", rc = 0 ? "completed successfully" : "failed");
+	snprintf(msg, sizeof(msg), "reset %s",
+		 rc = 0 ? "completed successfully" : "failed");
 	hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 	return rc = 0 ? SUCCESS : FAILED;
 }

walter harms

2015-06-04 15:22 UTC #1 lore raw

Am 04.06.2015 16:47, schrieb Dan Carpenter:

Quoted 44 lines

Fixes 73153fe533bc ('hpsa: use block layer tag for command allocation')

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

drivers/scsi/hpsa.c +7 -5

index 1dafeb4..cab4e98 100644
@@ -5104,7 +5104,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	int rc;
 	struct ctlr_info *h;
 	struct hpsa_scsi_dev_t *dev;
-	char msg[40];
+	char msg[48];
 
 	/* find the controller to which the command to be aborted was sent */
 	h = sdev_to_hba(scsicmd->device);

@@ -5122,16 +5122,18 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 
 	/* if controller locked up, we can guarantee command won't complete */
 	if (lockup_detected(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
 
 	/* this reset request might be the result of a lockup; check */
 	if (detect_controller_lockup(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, new lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, new lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}

drivers/scsi/hpsa.c static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd) · line 5154

Quoted 9 lines

drivers/scsi/hpsa.c +2 -1

@@ -5145,7 +5147,8 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	/* send a reset to the SCSI LUN which the command was sent to */
 	rc = hpsa_do_reset(h, dev, dev->scsi3addr, HPSA_RESET_TYPE_LUN,
 			   DEFAULT_REPLY_QUEUE);
-	sprintf(msg, "reset %s", rc = 0 ? "completed successfully" : "failed");
+	snprintf(msg, sizeof(msg), "reset %s",
+		 rc = 0 ? "completed successfully" : "failed");
 	hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 	return rc = 0 ? SUCCESS : FAILED;
 }

there is something called dev_printk_emit() what seems the varg version of dev_printk() (what is behind psa_show_dev_msg()) maybe it would be better to redefine the interface to use varargs ?

Its up to the maintainer to decide, i do not know how often hpsa_show_dev_msg is actualy used.

just my 2 cents,
wh

Don Brace

2015-06-18 13:35 UTC #2 lore raw

Quoted 77 lines

-----Original Message-----
From: walter harms [mailto:[email protected]]
Sent: Thursday, June 04, 2015 10:23 AM
To: Dan Carpenter
Cc: Don Brace; James E.J. Bottomley; [email protected]; dl Team ESD Storage Dev Support; [email protected]; kernel- [email protected]
Subject: Re: [patch] hpsa: fix an sprintf() overflow in the reset handler
Am 04.06.2015 16:47, schrieb Dan Carpenter:
drivers/scsi/hpsa.c static int hpsa_eh_device_reset_handler(struct
Quoted 12 lines
The string "cmd %d RESET FAILED, new lockup detected" is not quite large enough so the sprintf() will overflow. I have increased the size of the buffer and also changed the sprintf calls to snprintf.
Fixes 73153fe533bc ('hpsa: use block layer tag for command allocation')
Signed-off-by Dan Carpenter <[email protected]>
drivers/scsi/hpsa.c +0 -0
index 1dafeb4..cab4e98 100644
@@ -5104,7 +5104,7 @@ static int hpsa_eh_device_reset_handler(struct
scsi_cmnd *scsicmd)
drivers/scsi/hpsa.c static int hpsa_eh_device_reset_handler(struct
Quoted 9 lines
drivers/scsi/hpsa.c +1 -1
 	int rc;
 	struct ctlr_info *h;
 	struct hpsa_scsi_dev_t *dev;
-	char msg[40];
+	char msg[48];
 
 	/* find the controller to which the command to be aborted was sent */
 	h = sdev_to_hba(scsicmd->device);
@@ -5122,16 +5122,18 @@ static int hpsa_eh_device_reset_handler(struct
scsi_cmnd *scsicmd)
drivers/scsi/hpsa.c static int hpsa_eh_device_reset_handler(struct · line 5122
Quoted 2 lines
drivers/scsi/hpsa.c +0 -0
 	/* if controller locked up, we can guarantee command won't complete
*/
drivers/scsi/hpsa.c static int hpsa_eh_device_reset_handler(struct
Quoted 21 lines
drivers/scsi/hpsa.c +6 -4
 	if (lockup_detected(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
 
 	/* this reset request might be the result of a lockup; check */
 	if (detect_controller_lockup(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, new lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, new lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
@@ -5145,7 +5147,8 @@ static int hpsa_eh_device_reset_handler(struct
scsi_cmnd *scsicmd)
drivers/scsi/hpsa.c static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd) · line 5154
Quoted 9 lines
drivers/scsi/hpsa.c +2 -1
 	/* send a reset to the SCSI LUN which the command was sent to */
 	rc = hpsa_do_reset(h, dev, dev->scsi3addr, HPSA_RESET_TYPE_LUN,
 			   DEFAULT_REPLY_QUEUE);
-	sprintf(msg, "reset %s", rc = 0 ? "completed successfully" : "failed");
+	snprintf(msg, sizeof(msg), "reset %s",
+		 rc = 0 ? "completed successfully" : "failed");
 	hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 	return rc = 0 ? SUCCESS : FAILED;
 }
there is something called dev_printk_emit() what seems the varg version of dev_printk()
(what is behind psa_show_dev_msg()) maybe it would be better to redefine the interface
to use varargs ?

Quoted 6 lines

Its up to the maintainer to decide, i do not know how often hpsa_show_dev_msg is actualy used.
just my 2 cents,
wh

Thanks. I'll check into this and add it in as an update if necessary.

Don Brace

2015-06-18 13:35 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 64 lines

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, June 04, 2015 9:48 AM
To: Don Brace
Cc: James E.J. Bottomley; [email protected]; dl Team ESD Storage Dev Support; [email protected]; [email protected]
Subject: [patch] hpsa: fix an sprintf() overflow in the reset handler

Fixes 73153fe533bc ('hpsa: use block layer tag for command allocation')

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

drivers/scsi/hpsa.c +1 -1

index 1dafeb4..cab4e98 100644
@@ -5104,7 +5104,7 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 	int rc;
 	struct ctlr_info *h;
 	struct hpsa_scsi_dev_t *dev;
-	char msg[40];
+	char msg[48];
 
 	/* find the controller to which the command to be aborted was sent */

drivers/scsi/hpsa.c +6 -4

 	h = sdev_to_hba(scsicmd->device);

@@ -5122,16 +5122,18 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 
 	/* if controller locked up, we can guarantee command won't complete
 */
 	if (lockup_detected(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
 
 	/* this reset request might be the result of a lockup; check */
 	if (detect_controller_lockup(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, new lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, new lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);

drivers/scsi/hpsa.c +2 -1

 		return FAILED;
 	}

@@ -5145,7 +5147,8 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 	/* send a reset to the SCSI LUN which the command was sent to */
 	rc = hpsa_do_reset(h, dev, dev->scsi3addr, HPSA_RESET_TYPE_LUN,
 			   DEFAULT_REPLY_QUEUE);
-	sprintf(msg, "reset %s", rc = 0 ? "completed successfully" : "failed");
+	snprintf(msg, sizeof(msg), "reset %s",
+		 rc = 0 ? "completed successfully" : "failed");
 	hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 	return rc = 0 ? SUCCESS : FAILED;

Quoted 1 line

}

Signed-off-by Don Brace <[email protected]>

Seymour, Shane M

2015-06-19 07:13 UTC #4 lore raw

With a size of 48 while it won't overflow since you're using snprintf the string with a maximum value in %d:

echo -n "cmd 2147483647 RESET FAILED, new lockup detected" |wc -c 48

is 48 characters long without a null terminator on the string (and in the unlikely event that it's somehow a the largest possible negative value it would be 49 characters after including the minus sign without a null terminator). If you always want a complete message no matter what the value formatted as %d will be I believe it needs to have a length of 50. The worst that will happen now though because you're using snprintf is you'll drop the trailing "d" or "ed" in the message with very large positive or negative numbers.

My somewhat sketchy memory of the hpsa driver is that the nr_cmds member of the struct ctlr_info is never more than 1040 (?) anyway. If things are working correctly I don't think it should ever happen but I thought I should point out that msg isn't large enough to contain the complete contents of the longest possible character string.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Don Brace
Sent: Thursday, June 18, 2015 11:36 PM
To: Dan Carpenter
Cc: James E.J. Bottomley; ISS StorageDev; dl Team ESD Storage Dev Support; [email protected]; [email protected]
Subject: RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

Quoted 66 lines

Fixes 73153fe533bc ('hpsa: use block layer tag for command

allocation')

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

drivers/scsi/hpsa.c index renamed from drivers/scsi/hpsa.c +0 -0

1dafeb4..cab4e98 100644

drivers/scsi/hpsa.c +1 -1

@@ -5104,7 +5104,7 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 	int rc;
 	struct ctlr_info *h;
 	struct hpsa_scsi_dev_t *dev;
-	char msg[40];
+	char msg[48];
 
 	/* find the controller to which the command to be aborted was sent */

drivers/scsi/hpsa.c +6 -4

 	h = sdev_to_hba(scsicmd->device);

@@ -5122,16 +5122,18 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 
 	/* if controller locked up, we can guarantee command won't complete 
 */
 	if (lockup_detected(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		return FAILED;
 	}
 
 	/* this reset request might be the result of a lockup; check */
 	if (detect_controller_lockup(h)) {
-		sprintf(msg, "cmd %d RESET FAILED, new lockup detected",
-				hpsa_get_cmd_index(scsicmd));
+		snprintf(msg, sizeof(msg),
+			 "cmd %d RESET FAILED, new lockup detected",
+			 hpsa_get_cmd_index(scsicmd));
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);

drivers/scsi/hpsa.c +2 -1

 		return FAILED;
 	}

@@ -5145,7 +5147,8 @@ static int hpsa_eh_device_reset_handler(struct
 scsi_cmnd *scsicmd)
 	/* send a reset to the SCSI LUN which the command was sent to */
 	rc = hpsa_do_reset(h, dev, dev->scsi3addr, HPSA_RESET_TYPE_LUN,
 			   DEFAULT_REPLY_QUEUE);
-	sprintf(msg, "reset %s", rc = 0 ? "completed successfully" : "failed");
+	snprintf(msg, sizeof(msg), "reset %s",
+		 rc = 0 ? "completed successfully" : "failed");
 	hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 	return rc = 0 ? SUCCESS : FAILED;

Quoted 1 line

}

Signed-off-by Don Brace <[email protected]>

--
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

2015-06-24 11:01 UTC #5 lore raw

Quoted 4 lines · Shane M

With a size of 48 while it won't overflow since you're using snprintf the string with a maximum value in %d:
echo -n "cmd 2147483647 RESET FAILED, new lockup detected" |wc -c 48

I actually just chose 48 because it was divisible by sizeof(long) on 64 bit. The compiler is going to pad it out a bit anyway because of alignment.

Quoted 4 lines

is 48 characters long without a null terminator on the string (and in the unlikely event that it's somehow a the largest possible negative value it would be 49 characters after including the minus sign without a null terminator). If you always want a complete message no matter what the value formatted as %d will be I believe it needs to have a length of 50. The worst that will happen now though because you're using snprintf is you'll drop the trailing "d" or "ed" in the message with very large positive or negative numbers.
My somewhat sketchy memory of the hpsa driver is that the nr_cmds member of the struct ctlr_info is never more than 1040 (?) anyway. If things are working correctly I don't think it should ever happen but I thought I should point out that msg isn't large enough to contain the complete contents of the longest possible character string.

My knowledge is sketchier than yours.

When I was reading this I was thinking that instead of 1040 the upper limit was 32. I got that from hpsa_get_max_perf_mode_cmds(). The only negative number was -1, I think.

But either way, we both agree that 48 is probably safe.

regards,
dan carpenter