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
