* [PATCH] csiostor: Avoid content leaks and casts
@ 2017-05-09 22:34 Kees Cook
2017-05-19 1:35 ` Martin K. Petersen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kees Cook @ 2017-05-09 22:34 UTC (permalink / raw)
To: linux-kernel
Cc: James E.J. Bottomley, Martin K. Petersen, linux-scsi, Daniel Micay
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)
}
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;
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] csiostor: Avoid content leaks and casts
2017-05-09 22:34 [PATCH] csiostor: Avoid content leaks and casts Kees Cook
@ 2017-05-19 1:35 ` Martin K. Petersen
2017-05-22 15:05 ` Varun Prakash
2017-05-24 1:46 ` Martin K. Petersen
2 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2017-05-19 1:35 UTC (permalink / raw)
To: Varun Prakash
Cc: linux-kernel, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, Daniel Micay, Kees Cook
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/
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] csiostor: Avoid content leaks and casts
2017-05-09 22:34 [PATCH] csiostor: Avoid content leaks and casts Kees Cook
2017-05-19 1:35 ` Martin K. Petersen
@ 2017-05-22 15:05 ` Varun Prakash
2017-05-22 16:29 ` Kees Cook
2017-05-24 1:46 ` Martin K. Petersen
2 siblings, 1 reply; 6+ messages in thread
From: Varun Prakash @ 2017-05-22 15:05 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, Daniel Micay
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] csiostor: Avoid content leaks and casts
2017-05-22 15:05 ` Varun Prakash
@ 2017-05-22 16:29 ` Kees Cook
2017-05-23 9:30 ` Varun Prakash
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2017-05-22 16:29 UTC (permalink / raw)
To: Varun Prakash
Cc: LKML, James E.J. Bottomley, Martin K. Petersen, linux-scsi, Daniel Micay
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
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] csiostor: Avoid content leaks and casts
2017-05-22 16:29 ` Kees Cook
@ 2017-05-23 9:30 ` Varun Prakash
0 siblings, 0 replies; 6+ messages in thread
From: Varun Prakash @ 2017-05-23 9:30 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, James E.J. Bottomley, Martin K. Petersen, linux-scsi, Daniel Micay
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] csiostor: Avoid content leaks and casts
2017-05-09 22:34 [PATCH] csiostor: Avoid content leaks and casts Kees Cook
2017-05-19 1:35 ` Martin K. Petersen
2017-05-22 15:05 ` Varun Prakash
@ 2017-05-24 1:46 ` Martin K. Petersen
2 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2017-05-24 1:46 UTC (permalink / raw)
To: Kees Cook
Cc: linux-kernel, James E.J. Bottomley, Martin K. Petersen,
linux-scsi, Daniel Micay
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!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-24 1:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-09 22:34 [PATCH] csiostor: Avoid content leaks and casts Kees Cook
2017-05-19 1:35 ` Martin K. Petersen
2017-05-22 15:05 ` Varun Prakash
2017-05-22 16:29 ` Kees Cook
2017-05-23 9:30 ` Varun Prakash
2017-05-24 1:46 ` Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).