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

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

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 1dafeb4..cab4e98 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -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;
 }

Re: [patch] hpsa: fix an sprintf() overflow in the reset handler

[walter harms]

Am 04.06.2015 16:47, schrieb Dan Carpenter:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 1dafeb4..cab4e98 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -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;
>  }


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

RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

[Don Brace]

> -----Original Message-----
> From: walter harms [mailto:wharms@bfs.de]
> Sent: Thursday, June 04, 2015 10:23 AM
> To: Dan Carpenter
> Cc: Don Brace; James E.J. Bottomley; iss_storagedev@hp.com; dl Team ESD
> Storage Dev Support; linux-scsi@vger.kernel.org; kernel-
> janitors@vger.kernel.org
> Subject: Re: [patch] hpsa: fix an sprintf() overflow in the reset handler
> 
> 
> 
> Am 04.06.2015 16:47, schrieb Dan Carpenter:
> > 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 <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 1dafeb4..cab4e98 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -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;
> >  }
> 
> 
> 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
> 

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

RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

[Don Brace]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, June 04, 2015 9:48 AM
> To: Don Brace
> Cc: James E.J. Bottomley; iss_storagedev@hp.com; dl Team ESD Storage Dev
> Support; linux-scsi@vger.kernel.org; kernel-janitors@vger.kernel.org
> Subject: [patch] 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 1dafeb4..cab4e98 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -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;
>  }

Signed-off-by: Don Brace <don.brace@pmcs.com>

RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

[Seymour, 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

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: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] 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; linux-scsi@vger.kernel.org; kernel-janitors@vger.kernel.org
Subject: RE: [patch] hpsa: fix an sprintf() overflow in the reset handler

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, June 04, 2015 9:48 AM
> To: Don Brace
> Cc: James E.J. Bottomley; iss_storagedev@hp.com; dl Team ESD Storage 
> Dev Support; linux-scsi@vger.kernel.org; 
> kernel-janitors@vger.kernel.org
> Subject: [patch] 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 
> 1dafeb4..cab4e98 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -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;
>  }

Signed-off-by: Don Brace <don.brace@pmcs.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] hpsa: fix an sprintf() overflow in the reset handler

[Dan Carpenter]

On Fri, Jun 19, 2015 at 07:13:47AM +0000, Seymour, Shane M wrote:
> 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.

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