| Message ID | 20160915120142.GA2349@mwanda (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 15.9.2016 14:01, Dan Carpenter wrote: > We need to put an upper bound on "user_len" so the memcpy() doesn't > overflow. > > Reported-by: Marco Grassi <marco.gra@gmail.com> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c > index 7640498..73ddd45 100644 > --- a/drivers/scsi/arcmsr/arcmsr_hba.c > +++ b/drivers/scsi/arcmsr/arcmsr_hba.c > @@ -2388,7 +2388,8 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, > } > case ARCMSR_MESSAGE_WRITE_WQBUFFER: { > unsigned char *ver_addr; > - int32_t user_len, cnt2end; > + uint32_t user_len; > + int32_t cnt2end; > uint8_t *pQbuffer, *ptmpuserbuffer; > ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC); > if (!ver_addr) { > @@ -2397,6 +2398,10 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, > } > ptmpuserbuffer = ver_addr; > user_len = pcmdmessagefld->cmdmessage.Length; > + if (user_len > ARCMSR_API_DATA_BUFLEN) { > + retvalue = ARCMSR_MESSAGE_FAIL; Hi, I think that a 'kfree(ver_addr);' should be added here. With that you may add my reviewed-by. Tomas > + goto message_out; > + } > memcpy(ptmpuserbuffer, > pcmdmessagefld->messagedatabuffer, user_len); > spin_lock_irqsave(&acb->wqbuffer_lock, flags); > -- > 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 -- 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
On Thu, Sep 15, 2016 at 03:19:12PM +0200, Tomas Henzl wrote: > > @@ -2397,6 +2398,10 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, > > } > > ptmpuserbuffer = ver_addr; > > user_len = pcmdmessagefld->cmdmessage.Length; > > + if (user_len > ARCMSR_API_DATA_BUFLEN) { > > + retvalue = ARCMSR_MESSAGE_FAIL; > > Hi, > I think that a 'kfree(ver_addr);' should be added here. > With that you may add my reviewed-by. Oops... Thanks for catching that. I should have been more careful. v2 on the way. regards, dan carpenter -- 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
diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c index 7640498..73ddd45 100644 --- a/drivers/scsi/arcmsr/arcmsr_hba.c +++ b/drivers/scsi/arcmsr/arcmsr_hba.c @@ -2388,7 +2388,8 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, } case ARCMSR_MESSAGE_WRITE_WQBUFFER: { unsigned char *ver_addr; - int32_t user_len, cnt2end; + uint32_t user_len; + int32_t cnt2end; uint8_t *pQbuffer, *ptmpuserbuffer; ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC); if (!ver_addr) { @@ -2397,6 +2398,10 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, } ptmpuserbuffer = ver_addr; user_len = pcmdmessagefld->cmdmessage.Length; + if (user_len > ARCMSR_API_DATA_BUFLEN) { + retvalue = ARCMSR_MESSAGE_FAIL; + goto message_out; + } memcpy(ptmpuserbuffer, pcmdmessagefld->messagedatabuffer, user_len); spin_lock_irqsave(&acb->wqbuffer_lock, flags);
We need to put an upper bound on "user_len" so the memcpy() doesn't overflow. Reported-by: Marco Grassi <marco.gra@gmail.com> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.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