Kees, On Tue, Feb 14, 2023 at 04:08:39PM -0800, Kees Cook wrote: > The kernel is globally removing the ambiguous 0-length and 1-element > arrays in favor of flexible arrays, so that we can gain both compile-time > and run-time array bounds checking[1]. > > While struct fealist is defined as a "fake" flexible array (via a > 1-element array), it is only used for examination of the first array > element. Walking the list is performed separately, so there is no reason > to treat the "list" member of struct fealist as anything other than a > single entry. Adjust the struct and code to match. > > Additionally, struct fea uses the "name" member either as a dynamic > string, or is manually calculated from the start of the struct. Redefine > the member as a flexible array. > > No machine code output differences are produced after these changes. > > [1] For lots of details, see both: > https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays > https://people.kernel.org/kees/bounded-flexible-arrays-in-c > > Cc: Steve French <sfrench@xxxxxxxxx> > Cc: Paulo Alcantara <pc@xxxxxx> > Cc: Ronnie Sahlberg <lsahlber@xxxxxxxxxx> > Cc: Shyam Prasad N <sprasad@xxxxxxxxxxxxx> > Cc: Tom Talpey <tom@xxxxxxxxxx> > Cc: linux-cifs@xxxxxxxxxxxxxxx > Cc: samba-technical@xxxxxxxxxxxxxxx > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > --- > fs/cifs/cifspdu.h | 4 ++-- > fs/cifs/cifssmb.c | 16 ++++++++-------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h > index 623caece2b10..add73be4902c 100644 > --- a/fs/cifs/cifspdu.h > +++ b/fs/cifs/cifspdu.h > @@ -2583,7 +2583,7 @@ struct fea { > unsigned char EA_flags; > __u8 name_len; > __le16 value_len; > - char name[1]; > + char name[]; > /* optionally followed by value */ > } __attribute__((packed)); > /* flags for _FEA.fEA */ > @@ -2591,7 +2591,7 @@ struct fea { > > struct fealist { > __le32 list_len; > - struct fea list[1]; > + struct fea list; > } __attribute__((packed)); > > /* used to hold an arbitrary blob of data */ > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index 60dd4e37030a..7c587157d030 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -5787,7 +5787,7 @@ CIFSSMBQAllEAs(const unsigned int xid, struct cifs_tcon *tcon, > > /* account for ea list len */ > list_len -= 4; > - temp_fea = ea_response_data->list; > + temp_fea = &ea_response_data->list; > temp_ptr = (char *)temp_fea; > while (list_len > 0) { > unsigned int name_len; > @@ -5902,7 +5902,7 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, > else > name_len = strnlen(ea_name, 255); > > - count = sizeof(*parm_data) + ea_value_len + name_len; > + count = sizeof(*parm_data) + 1 + ea_value_len + name_len; > pSMB->MaxParameterCount = cpu_to_le16(2); > /* BB find max SMB PDU from sess */ > pSMB->MaxDataCount = cpu_to_le16(1000); > @@ -5926,14 +5926,14 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, > byte_count = 3 /* pad */ + params + count; > pSMB->DataCount = cpu_to_le16(count); > parm_data->list_len = cpu_to_le32(count); > - parm_data->list[0].EA_flags = 0; > + parm_data->list.EA_flags = 0; > /* we checked above that name len is less than 255 */ > - parm_data->list[0].name_len = (__u8)name_len; > + parm_data->list.name_len = (__u8)name_len; > /* EA names are always ASCII */ > if (ea_name) > - strncpy(parm_data->list[0].name, ea_name, name_len); > - parm_data->list[0].name[name_len] = 0; > - parm_data->list[0].value_len = cpu_to_le16(ea_value_len); > + strncpy(parm_data->list.name, ea_name, name_len); Could non-applying this patch cause false-positive fortify_panic? We got a bug report from user of 6.1.73: Jan 24 15:15:20 kalt2test.dpt.local kernel: detected buffer overflow in strncpy Jan 24 15:15:20 kalt2test.dpt.local kernel: ------------[ cut here ]------------ Jan 24 15:15:20 kalt2test.dpt.local kernel: kernel BUG at lib/string_helpers.c:1027! ... Jan 24 15:15:20 kalt2test.dpt.local kernel: Call Trace: Jan 24 15:15:20 kalt2test.dpt.local kernel: <TASK> Jan 24 15:15:20 kalt2test.dpt.local kernel: ? __die_body.cold+0x1a/0x1f Jan 24 15:15:20 kalt2test.dpt.local kernel: ? die+0x2b/0x50 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? do_trap+0xcf/0x120 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? fortify_panic+0xf/0x11 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? do_error_trap+0x83/0xb0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? fortify_panic+0xf/0x11 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? exc_invalid_op+0x4e/0x70 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? fortify_panic+0xf/0x11 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? asm_exc_invalid_op+0x16/0x20 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? fortify_panic+0xf/0x11 Jan 24 15:15:20 kalt2test.dpt.local kernel: CIFSSMBSetEA.cold+0xc/0x18 [cifs] Jan 24 15:15:20 kalt2test.dpt.local kernel: cifs_xattr_set+0x596/0x690 [cifs] Jan 24 15:15:20 kalt2test.dpt.local kernel: ? evm_protected_xattr_common+0x41/0xb0 Jan 24 15:15:20 kalt2test.dpt.local kernel: __vfs_removexattr+0x52/0x70 Jan 24 15:15:20 kalt2test.dpt.local kernel: __vfs_removexattr_locked+0xbc/0x150 Jan 24 15:15:20 kalt2test.dpt.local kernel: vfs_removexattr+0x56/0x100 Jan 24 15:15:20 kalt2test.dpt.local kernel: removexattr+0x58/0x90 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? get_vtime_delta+0xf/0xb0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? ct_kernel_exit.constprop.0+0x6b/0x80 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? __ct_user_enter+0x5a/0xd0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? syscall_exit_to_user_mode+0x31/0x50 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? int80_emulation+0xb9/0x110 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? get_vtime_delta+0xf/0xb0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? ct_kernel_exit.constprop.0+0x6b/0x80 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? __ct_user_enter+0x5a/0xd0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? __fget_light.part.0+0x83/0xd0 Jan 24 15:15:20 kalt2test.dpt.local kernel: __ia32_sys_fremovexattr+0x80/0xa0 Jan 24 15:15:20 kalt2test.dpt.local kernel: int80_emulation+0xa9/0x110 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? get_vtime_delta+0xf/0xb0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? vtime_user_exit+0x1c/0x70 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? __ct_user_exit+0x6c/0xc0 Jan 24 15:15:20 kalt2test.dpt.local kernel: ? int80_emulation+0x1b/0x110 Jan 24 15:15:20 kalt2test.dpt.local kernel: asm_int80_emulation+0x16/0x20 I don't find this patch appled to stable/linux-6.1.y. Thanks, ps. (Unfortunately `CIFSSMBSetEA+0xc` address is not resolvable to the actual line inside of CIFSSMBSetEA pointing just to the head of it. (gdb) l *CIFSSMBSetEA+0xc 0x6de3c is in CIFSSMBSetEA (fs/smb/client/cifssmb.c:5776). 5771 int 5772 CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, 5773 const char *fileName, const char *ea_name, const void *ea_value, 5774 const __u16 ea_value_len, const struct nls_table *nls_codepage, 5775 struct cifs_sb_info *cifs_sb) 5776 { 5777 struct smb_com_transaction2_spi_req *pSMB = NULL; 5778 struct smb_com_transaction2_spi_rsp *pSMBr = NULL; 5779 struct fealist *parm_data; 5780 int name_len; But there is only one strncpy there. > + parm_data->list.name[name_len] = '\0'; > + parm_data->list.value_len = cpu_to_le16(ea_value_len); > /* caller ensures that ea_value_len is less than 64K but > we need to ensure that it fits within the smb */ > > @@ -5941,7 +5941,7 @@ CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, > negotiated SMB buffer size BB */ > /* if (ea_value_len > buffer_size - 512 (enough for header)) */ > if (ea_value_len) > - memcpy(parm_data->list[0].name+name_len+1, > + memcpy(parm_data->list.name + name_len + 1, > ea_value, ea_value_len); > > pSMB->TotalDataCount = pSMB->DataCount; > -- > 2.34.1 >
