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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: walter harms [mailto:wharms@xxxxxx]
> Sent: Thursday, June 04, 2015 10:23 AM
> To: Dan Carpenter
> Cc: Don Brace; James E.J. Bottomley; iss_storagedev@xxxxxx; dl Team ESD
> Storage Dev Support; linux-scsi@xxxxxxxxxxxxxxx; kernel-
> janitors@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> >
> > 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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux