| Message ID | 20170509223444.GA51314@beast (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Varun, You weren't CC:ed on this patch. Please review. Thanks! > When copying attributes, the len argument was padded out and the > resulting memcpy() would copy beyond the end of the source buffer. > Avoid this, and use size_t for val_len to avoid all the > casts. Similarly, avoid source buffer casts and use void *. > > Additionally enforces val_len can be represented by u16 and that the > DMA buffer was not overflowed. Fixes the size of mfa, which is not > FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to > 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks. https://patchwork.kernel.org/patch/9718995/
On Tue, May 09, 2017 at 03:34:44PM -0700, Kees Cook wrote: > When copying attributes, the len argument was padded out and the resulting > memcpy() would copy beyond the end of the source buffer. Avoid this, > and use size_t for val_len to avoid all the casts. Similarly, avoid source > buffer casts and use void *. > > Additionally enforces val_len can be represented by u16 and that > the DMA buffer was not overflowed. Fixes the size of mfa, which is not > FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This > was noticed by the future CONFIG_FORTIFY_SOURCE checks. > > Cc: Daniel Micay <danielmicay@gmail.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/scsi/csiostor/csio_lnode.c b/drivers/scsi/csiostor/csio_lnode.c > index c00b2ff72b55..be5ee2d37815 100644 > --- a/drivers/scsi/csiostor/csio_lnode.c > +++ b/drivers/scsi/csiostor/csio_lnode.c > @@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len) > } > > > csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD, > - (uint8_t *)&maxpayload, > - FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); > + &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); > len = (uint32_t)(pld - (uint8_t *)cmd); > numattrs++; > attrib_blk->numattrs = htonl(numattrs); > @@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, > struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw); > int rv; > > + BUG_ON(pld_len > pld->len); > + I think WARN_ON() is better than BUG_ON() in this case if (WARN_ON(pld_len > pld->len)) return -EINVAL; > io_req->io_cbfn = io_cbfn; /* Upper layer callback handler */ > io_req->fw_handle = (uintptr_t) (io_req); > io_req->eq_idx = mgmtm->eq_idx;
On Mon, May 22, 2017 at 8:05 AM, Varun Prakash <varun@chelsio.com> wrote: > On Tue, May 09, 2017 at 03:34:44PM -0700, Kees Cook wrote: >> When copying attributes, the len argument was padded out and the resulting >> memcpy() would copy beyond the end of the source buffer. Avoid this, >> and use size_t for val_len to avoid all the casts. Similarly, avoid source >> buffer casts and use void *. >> >> Additionally enforces val_len can be represented by u16 and that >> the DMA buffer was not overflowed. Fixes the size of mfa, which is not >> FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This >> was noticed by the future CONFIG_FORTIFY_SOURCE checks. >> >> Cc: Daniel Micay <danielmicay@gmail.com> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++--------------- >> 1 file changed, 26 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/scsi/csiostor/csio_lnode.c b/drivers/scsi/csiostor/csio_lnode.c >> index c00b2ff72b55..be5ee2d37815 100644 >> --- a/drivers/scsi/csiostor/csio_lnode.c >> +++ b/drivers/scsi/csiostor/csio_lnode.c >> @@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len) >> } >> > > >> >> csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD, >> - (uint8_t *)&maxpayload, >> - FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); >> + &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); >> len = (uint32_t)(pld - (uint8_t *)cmd); >> numattrs++; >> attrib_blk->numattrs = htonl(numattrs); >> @@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, >> struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw); >> int rv; >> >> + BUG_ON(pld_len > pld->len); >> + > > I think WARN_ON() is better than BUG_ON() in this case > > if (WARN_ON(pld_len > pld->len)) > return -EINVAL; > >> io_req->io_cbfn = io_cbfn; /* Upper layer callback handler */ >> io_req->fw_handle = (uintptr_t) (io_req); >> io_req->eq_idx = mgmtm->eq_idx; I chose BUG_ON here because the damage has already been done. If this assertion is hit, the heap buffers have already been overrun. This isn't a state we should only warn about... -Kees
On Mon, May 22, 2017 at 09:29:41AM -0700, Kees Cook wrote: > On Mon, May 22, 2017 at 8:05 AM, Varun Prakash <varun@chelsio.com> wrote: > > On Tue, May 09, 2017 at 03:34:44PM -0700, Kees Cook wrote: > >> When copying attributes, the len argument was padded out and the resulting > >> memcpy() would copy beyond the end of the source buffer. Avoid this, > >> and use size_t for val_len to avoid all the casts. Similarly, avoid source > >> buffer casts and use void *. > >> > >> Additionally enforces val_len can be represented by u16 and that > >> the DMA buffer was not overflowed. Fixes the size of mfa, which is not > >> FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This > >> was noticed by the future CONFIG_FORTIFY_SOURCE checks. > >> > >> Cc: Daniel Micay <danielmicay@gmail.com> > >> Signed-off-by: Kees Cook <keescook@chromium.org> > >> --- > >> drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++--------------- > >> 1 file changed, 26 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/scsi/csiostor/csio_lnode.c b/drivers/scsi/csiostor/csio_lnode.c > >> index c00b2ff72b55..be5ee2d37815 100644 > >> --- a/drivers/scsi/csiostor/csio_lnode.c > >> +++ b/drivers/scsi/csiostor/csio_lnode.c > >> @@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len) > >> } > >> > > > > > >> > >> csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD, > >> - (uint8_t *)&maxpayload, > >> - FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); > >> + &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); > >> len = (uint32_t)(pld - (uint8_t *)cmd); > >> numattrs++; > >> attrib_blk->numattrs = htonl(numattrs); > >> @@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, > >> struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw); > >> int rv; > >> > >> + BUG_ON(pld_len > pld->len); > >> + > > > > I think WARN_ON() is better than BUG_ON() in this case > > > > if (WARN_ON(pld_len > pld->len)) > > return -EINVAL; > > > >> io_req->io_cbfn = io_cbfn; /* Upper layer callback handler */ > >> io_req->fw_handle = (uintptr_t) (io_req); > >> io_req->eq_idx = mgmtm->eq_idx; > > I chose BUG_ON here because the damage has already been done. If this > assertion is hit, the heap buffers have already been overrun. This > isn't a state we should only warn about... > Ok. Acked-by: Varun Prakash <varun@chelsio.com>
Kees, > When copying attributes, the len argument was padded out and the > resulting memcpy() would copy beyond the end of the source buffer. > Avoid this, and use size_t for val_len to avoid all the > casts. Similarly, avoid source buffer casts and use void *. > > Additionally enforces val_len can be represented by u16 and that the > DMA buffer was not overflowed. Fixes the size of mfa, which is not > FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to > 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks. Applied to 4.13/scsi-queue, thanks!
diff --git a/drivers/scsi/csiostor/csio_lnode.c b/drivers/scsi/csiostor/csio_lnode.c index c00b2ff72b55..be5ee2d37815 100644 --- a/drivers/scsi/csiostor/csio_lnode.c +++ b/drivers/scsi/csiostor/csio_lnode.c @@ -238,14 +238,23 @@ csio_osname(uint8_t *buf, size_t buf_len) } static inline void -csio_append_attrib(uint8_t **ptr, uint16_t type, uint8_t *val, uint16_t len) +csio_append_attrib(uint8_t **ptr, uint16_t type, void *val, size_t val_len) { + uint16_t len; struct fc_fdmi_attr_entry *ae = (struct fc_fdmi_attr_entry *)*ptr; + + if (WARN_ON(val_len > U16_MAX)) + return; + + len = val_len; + ae->type = htons(type); len += 4; /* includes attribute type and length */ len = (len + 3) & ~3; /* should be multiple of 4 bytes */ ae->len = htons(len); - memcpy(ae->value, val, len); + memcpy(ae->value, val, val_len); + if (len > val_len) + memset(ae->value + val_len, 0, len - val_len); *ptr += len; } @@ -335,7 +344,7 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req) numattrs++; val = htonl(FC_PORTSPEED_1GBIT | FC_PORTSPEED_10GBIT); csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_SUPPORTEDSPEED, - (uint8_t *)&val, + &val, FC_FDMI_PORT_ATTR_SUPPORTEDSPEED_LEN); numattrs++; @@ -346,23 +355,22 @@ csio_ln_fdmi_rhba_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req) else val = htonl(CSIO_HBA_PORTSPEED_UNKNOWN); csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED, - (uint8_t *)&val, - FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN); + &val, FC_FDMI_PORT_ATTR_CURRENTPORTSPEED_LEN); numattrs++; mfs = ln->ln_sparm.csp.sp_bb_data; csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_MAXFRAMESIZE, - (uint8_t *)&mfs, FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN); + &mfs, sizeof(mfs)); numattrs++; strcpy(buf, "csiostor"); csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_OSDEVICENAME, buf, - (uint16_t)strlen(buf)); + strlen(buf)); numattrs++; if (!csio_hostname(buf, sizeof(buf))) { csio_append_attrib(&pld, FC_FDMI_PORT_ATTR_HOSTNAME, - buf, (uint16_t)strlen(buf)); + buf, strlen(buf)); numattrs++; } attrib_blk->numattrs = htonl(numattrs); @@ -444,33 +452,32 @@ csio_ln_fdmi_dprt_cbfn(struct csio_hw *hw, struct csio_ioreq *fdmi_req) strcpy(buf, "Chelsio Communications"); csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MANUFACTURER, buf, - (uint16_t)strlen(buf)); + strlen(buf)); numattrs++; csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_SERIALNUMBER, - hw->vpd.sn, (uint16_t)sizeof(hw->vpd.sn)); + hw->vpd.sn, sizeof(hw->vpd.sn)); numattrs++; csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MODEL, hw->vpd.id, - (uint16_t)sizeof(hw->vpd.id)); + sizeof(hw->vpd.id)); numattrs++; csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MODELDESCRIPTION, - hw->model_desc, (uint16_t)strlen(hw->model_desc)); + hw->model_desc, strlen(hw->model_desc)); numattrs++; csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_HARDWAREVERSION, - hw->hw_ver, (uint16_t)sizeof(hw->hw_ver)); + hw->hw_ver, sizeof(hw->hw_ver)); numattrs++; csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_FIRMWAREVERSION, - hw->fwrev_str, (uint16_t)strlen(hw->fwrev_str)); + hw->fwrev_str, strlen(hw->fwrev_str)); numattrs++; if (!csio_osname(buf, sizeof(buf))) { csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_OSNAMEVERSION, - buf, (uint16_t)strlen(buf)); + buf, strlen(buf)); numattrs++; } csio_append_attrib(&pld, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD, - (uint8_t *)&maxpayload, - FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); + &maxpayload, FC_FDMI_HBA_ATTR_MAXCTPAYLOAD_LEN); len = (uint32_t)(pld - (uint8_t *)cmd); numattrs++; attrib_blk->numattrs = htonl(numattrs); @@ -1794,6 +1801,8 @@ csio_ln_mgmt_submit_req(struct csio_ioreq *io_req, struct csio_mgmtm *mgmtm = csio_hw_to_mgmtm(hw); int rv; + BUG_ON(pld_len > pld->len); + io_req->io_cbfn = io_cbfn; /* Upper layer callback handler */ io_req->fw_handle = (uintptr_t) (io_req); io_req->eq_idx = mgmtm->eq_idx;
When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *. Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks. Cc: Daniel Micay <danielmicay@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/scsi/csiostor/csio_lnode.c | 43 +++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 17 deletions(-)