Re: startup BUG at lib/string_helpers.c from scsi fusion mptsas

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux