On Thu, 2024-04-04 at 15:04 -0700, Justin Stitt wrote:
> Hi,
>
> On Thu, Apr 4, 2024 at 2:53 PM James Bottomley
> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 2024-04-04 at 14:38 -0700, Justin Stitt wrote:
> > [...]
> > > I am not sure how my patch [1] is triggering this fortify panic.
> > > I
> > > didn't modify this printk or the string arguments (ioc->name),
> > > also
> > > the change from strncpy to strscpy did not introduce any
> > > strnlen()'s
> > > which seems to be the thing fortify is upset about:
> > > "2024-04-01T19:18:28.000000+00:00 zGMT kernel - - - detected
> > > buffer
> > > overflow in strnlen"
> > > or
> > > "2024-04-01T22:23:45.000000+00:00 zGMT kernel - - - strnlen:
> > > detected
> > > buffer overflow: 9 byte read of buffer size 8"
> >
> > it's sitting in the definition of sized_strscpy in fortify-string.h
>
> I see now, I was jumping to the definition in lib/...
>
> >
> > Since the fields in question aren't zero terminated there's a bad
> > assumption that you can do strnlen on the source field.
>
> It's interesting that fortify doesn't like if the len argument is one
> byte larger than the source argument because while technically it
> does
> overread the buffer strscpy will manually write a NUL-byte to the
> destination buffer.
>
> The easy fix I see that doesn't limit the amount of representable
> data
> is to increase these fields by 1 to match sas_expander_device.
>
> diff --git a/drivers/message/fusion/mptsas.c
> b/drivers/message/fusion/mptsas.c
> index 300f8e955a53..44b492698e06 100644
> --- a/drivers/message/fusion/mptsas.c
> +++ b/drivers/message/fusion/mptsas.c
> @@ -2833,10 +2833,10 @@ struct rep_manu_reply{
> u8 sas_format:1;
> u8 reserved1:7;
> u8 reserved2[3];
> - u8 vendor_id[SAS_EXPANDER_VENDOR_ID_LEN];
> - u8 product_id[SAS_EXPANDER_PRODUCT_ID_LEN];
> - u8 product_rev[SAS_EXPANDER_PRODUCT_REV_LEN];
> - u8 component_vendor_id[SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN];
> + u8 vendor_id[SAS_EXPANDER_VENDOR_ID_LEN+1];
> + u8 product_id[SAS_EXPANDER_PRODUCT_ID_LEN+1];
> + u8 product_rev[SAS_EXPANDER_PRODUCT_REV_LEN+1];
> + u8 component_vendor_id[SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN+1];
This is a reply returned by the firmware ... we can't just arbitrarily
change field sizes because then it won't match what the firmware is
sending.
James
