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];
u16 component_id;
u8 component_revision_id;
u8 reserved3;
which matches:
struct sas_expander_device {
...
#define SAS_EXPANDER_VENDOR_ID_LEN 8
char vendor_id[SAS_EXPANDER_VENDOR_ID_LEN+1];
#define SAS_EXPANDER_PRODUCT_ID_LEN 16
char product_id[SAS_EXPANDER_PRODUCT_ID_LEN+1];
#define SAS_EXPANDER_PRODUCT_REV_LEN 4
char product_rev[SAS_EXPANDER_PRODUCT_REV_LEN+1];
#define SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN 8
char component_vendor_id[SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN+1];
...
If this is A-OK, I'll send a patch.
>
> James
>
Thanks
Justin
