| Message ID | 20160923112226.rteir5ivjou64ffh@pd.tnic (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Fri, Sep 23, 2016 at 01:22:26PM +0200, Borislav Petkov wrote: > On Thu, Sep 15, 2016 at 09:59:01AM -0400, Martin K. Petersen wrote: > > >>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes: > > > > Dan> We need to put an upper bound on "user_len" so the memcpy() doesn't > > Dan> overflow. > > > > Applied to 4.9/scsi-queue. > > Yap, Tomas said the kfree was missing on the error path but can we > simplify this further by doing the user_len check first so that the > kfree() is not even needed? > > Patch ontop: > > --- > From: Borislav Petkov <bp@suse.de> > Date: Fri, 23 Sep 2016 13:04:51 +0200 > Subject: [PATCH] scsi: arcmsr: Simplify user_len checking > > Do the user_len check first and then the ver_addr allocation so that > we can save us the kfree() on the error path when user_len is > > ARCMSR_API_DATA_BUFLEN. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Cc: Marco Grassi <marco.gra@gmail.com> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Tomas Henzl <thenzl@redhat.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > --- Looks good to me, Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On 23.9.2016 13:22, Borislav Petkov wrote: > On Thu, Sep 15, 2016 at 09:59:01AM -0400, Martin K. Petersen wrote: >>>>>>> "Dan" == Dan Carpenter <dan.carpenter@oracle.com> writes: >> Dan> We need to put an upper bound on "user_len" so the memcpy() doesn't >> Dan> overflow. >> >> Applied to 4.9/scsi-queue. > Yap, Tomas said the kfree was missing on the error path but can we > simplify this further by doing the user_len check first so that the > kfree() is not even needed? > > Patch ontop: > > --- > From: Borislav Petkov <bp@suse.de> > Date: Fri, 23 Sep 2016 13:04:51 +0200 > Subject: [PATCH] scsi: arcmsr: Simplify user_len checking > > Do the user_len check first and then the ver_addr allocation so that > we can save us the kfree() on the error path when user_len is > > ARCMSR_API_DATA_BUFLEN. > > Signed-off-by: Borislav Petkov <bp@suse.de> > Cc: Marco Grassi <marco.gra@gmail.com> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Tomas Henzl <thenzl@redhat.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> Looks good, Reviewed-by: Tomas Henzl <thenzl@redhat.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
>>>>> "Borislav" == Borislav Petkov <bp@alien8.de> writes:
Borislav> Yap, Tomas said the kfree was missing on the error path but
Borislav> can we simplify this further by doing the user_len check first
Borislav> so that the kfree() is not even needed?
Applied to 4.9/scsi-queue.
diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c index 110eca9eaca0..3d53d636b17b 100644 --- a/drivers/scsi/arcmsr/arcmsr_hba.c +++ b/drivers/scsi/arcmsr/arcmsr_hba.c @@ -2391,18 +2391,20 @@ static int arcmsr_iop_message_xfer(struct AdapterControlBlock *acb, uint32_t user_len; int32_t cnt2end; uint8_t *pQbuffer, *ptmpuserbuffer; - ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC); - if (!ver_addr) { + + user_len = pcmdmessagefld->cmdmessage.Length; + if (user_len > ARCMSR_API_DATA_BUFLEN) { retvalue = ARCMSR_MESSAGE_FAIL; goto message_out; } - ptmpuserbuffer = ver_addr; - user_len = pcmdmessagefld->cmdmessage.Length; - if (user_len > ARCMSR_API_DATA_BUFLEN) { + + ver_addr = kmalloc(ARCMSR_API_DATA_BUFLEN, GFP_ATOMIC); + if (!ver_addr) { retvalue = ARCMSR_MESSAGE_FAIL; - kfree(ver_addr); goto message_out; } + ptmpuserbuffer = ver_addr; + memcpy(ptmpuserbuffer, pcmdmessagefld->messagedatabuffer, user_len); spin_lock_irqsave(&acb->wqbuffer_lock, flags);