* [patch] arcmsr: buffer overflow in arcmsr_iop_message_xfer() [not found] <CAFkTriJHaO0-O0AXKWSZW6bXT_2m+VqRdwshFsgeq1=Bjfn_OQ@mail.gmail.com> @ 2016-09-15 12:01 ` Dan Carpenter 2016-09-15 13:19 ` Tomas Henzl 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2016-09-15 12:01 UTC (permalink / raw) To: James E.J. Bottomley, Marco Grassi Cc: Martin K. Petersen, Ching Huang, Johannes Thumshirn, Hannes Reinicke, Tomas Henzl, linux-scsi, security 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; + goto message_out; + } memcpy(ptmpuserbuffer, pcmdmessagefld->messagedatabuffer, user_len); spin_lock_irqsave(&acb->wqbuffer_lock, flags); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-15 12:01 ` [patch] arcmsr: buffer overflow in arcmsr_iop_message_xfer() Dan Carpenter @ 2016-09-15 13:19 ` Tomas Henzl 2016-09-15 13:43 ` Dan Carpenter 2016-09-15 13:44 ` [patch v2] " Dan Carpenter 0 siblings, 2 replies; 9+ messages in thread From: Tomas Henzl @ 2016-09-15 13:19 UTC (permalink / raw) To: Dan Carpenter, James E.J. Bottomley, Marco Grassi Cc: Martin K. Petersen, Ching Huang, Johannes Thumshirn, Hannes Reinicke, linux-scsi, security 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-15 13:19 ` Tomas Henzl @ 2016-09-15 13:43 ` Dan Carpenter 2016-09-15 13:44 ` [patch v2] " Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2016-09-15 13:43 UTC (permalink / raw) To: Tomas Henzl Cc: James E.J. Bottomley, Marco Grassi, Martin K. Petersen, Ching Huang, Johannes Thumshirn, Hannes Reinicke, linux-scsi, security 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch v2] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-15 13:19 ` Tomas Henzl 2016-09-15 13:43 ` Dan Carpenter @ 2016-09-15 13:44 ` Dan Carpenter 2016-09-15 13:59 ` Martin K. Petersen 1 sibling, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2016-09-15 13:44 UTC (permalink / raw) To: James E.J. Bottomley Cc: Martin K. Petersen, Ching Huang, Hannes Reinicke, Johannes Thumshirn, Tomas Henzl, linux-scsi, security 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> Reviewed-by: Tomas Henzl <thenzl@redhat.com> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c index 7640498..110eca9 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,11 @@ 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; + kfree(ver_addr); + goto message_out; + } memcpy(ptmpuserbuffer, pcmdmessagefld->messagedatabuffer, user_len); spin_lock_irqsave(&acb->wqbuffer_lock, flags); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch v2] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-15 13:44 ` [patch v2] " Dan Carpenter @ 2016-09-15 13:59 ` Martin K. Petersen 2016-09-23 11:22 ` Borislav Petkov 0 siblings, 1 reply; 9+ messages in thread From: Martin K. Petersen @ 2016-09-15 13:59 UTC (permalink / raw) To: Dan Carpenter Cc: James E.J. Bottomley, Martin K. Petersen, Ching Huang, Hannes Reinicke, Johannes Thumshirn, Tomas Henzl, linux-scsi, security >>>>> "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. -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-15 13:59 ` Martin K. Petersen @ 2016-09-23 11:22 ` Borislav Petkov 2016-09-26 11:58 ` Johannes Thumshirn ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Borislav Petkov @ 2016-09-23 11:22 UTC (permalink / raw) To: Martin K. Petersen Cc: Dan Carpenter, James E.J. Bottomley, Ching Huang, Hannes Reinicke, Johannes Thumshirn, Tomas Henzl, linux-scsi, security 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> --- drivers/scsi/arcmsr/arcmsr_hba.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) 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); -- 1.8.5.2 -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch v2] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-23 11:22 ` Borislav Petkov @ 2016-09-26 11:58 ` Johannes Thumshirn 2016-09-26 13:29 ` Tomas Henzl 2016-09-27 1:08 ` Martin K. Petersen 2 siblings, 0 replies; 9+ messages in thread From: Johannes Thumshirn @ 2016-09-26 11:58 UTC (permalink / raw) To: Borislav Petkov Cc: Martin K. Petersen, Dan Carpenter, James E.J. Bottomley, Ching Huang, Hannes Reinicke, Tomas Henzl, linux-scsi, security 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> -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-23 11:22 ` Borislav Petkov 2016-09-26 11:58 ` Johannes Thumshirn @ 2016-09-26 13:29 ` Tomas Henzl 2016-09-27 1:08 ` Martin K. Petersen 2 siblings, 0 replies; 9+ messages in thread From: Tomas Henzl @ 2016-09-26 13:29 UTC (permalink / raw) To: Borislav Petkov, Martin K. Petersen Cc: Dan Carpenter, James E.J. Bottomley, Ching Huang, Hannes Reinicke, Johannes Thumshirn, linux-scsi, security 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch v2] arcmsr: buffer overflow in arcmsr_iop_message_xfer() 2016-09-23 11:22 ` Borislav Petkov 2016-09-26 11:58 ` Johannes Thumshirn 2016-09-26 13:29 ` Tomas Henzl @ 2016-09-27 1:08 ` Martin K. Petersen 2 siblings, 0 replies; 9+ messages in thread From: Martin K. Petersen @ 2016-09-27 1:08 UTC (permalink / raw) To: Borislav Petkov Cc: Martin K. Petersen, Dan Carpenter, James E.J. Bottomley, Ching Huang, Hannes Reinicke, Johannes Thumshirn, Tomas Henzl, linux-scsi, security >>>>> "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. -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-09-27 1:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAFkTriJHaO0-O0AXKWSZW6bXT_2m+VqRdwshFsgeq1=Bjfn_OQ@mail.gmail.com>
2016-09-15 12:01 ` [patch] arcmsr: buffer overflow in arcmsr_iop_message_xfer() Dan Carpenter
2016-09-15 13:19 ` Tomas Henzl
2016-09-15 13:43 ` Dan Carpenter
2016-09-15 13:44 ` [patch v2] " Dan Carpenter
2016-09-15 13:59 ` Martin K. Petersen
2016-09-23 11:22 ` Borislav Petkov
2016-09-26 11:58 ` Johannes Thumshirn
2016-09-26 13:29 ` Tomas Henzl
2016-09-27 1:08 ` Martin K. Petersen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.