Re: [patch] hpsa: fix an sprintf() overflow in the reset handler
On Fri, Jun 19, 2015 at 07:13:47AM +, 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 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@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
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-ow...@vger.kernel.org
[mailto:linux-scsi-ow...@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-janit...@vger.kernel.org
Subject: RE: [patch] hpsa: fix an sprintf() overflow in the reset handler
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Thursday, June 04, 2015 9:48 AM
To: Don Brace
Cc: James E.J. Bottomley; iss_storage...@hp.com; dl Team ESD Storage
Dev Support; linux-scsi@vger.kernel.org;
kernel-janit...@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.carpen...@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.br...@pmcs.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in the
body of a message to majord...@vger.kernel.org More majordomo info at
http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@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
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Thursday, June 04, 2015 9:48 AM
To: Don Brace
Cc: James E.J. Bottomley; iss_storage...@hp.com; dl Team ESD Storage Dev
Support; linux-scsi@vger.kernel.org; kernel-janit...@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.carpen...@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.br...@pmcs.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@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
-Original Message-
From: walter harms [mailto:wha...@bfs.de]
Sent: Thursday, June 04, 2015 10:23 AM
To: Dan Carpenter
Cc: Don Brace; James E.J. Bottomley; iss_storage...@hp.com; dl Team ESD
Storage Dev Support; linux-scsi@vger.kernel.org; kernel-
janit...@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.carpen...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@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
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.carpen...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
