| Message ID | 20230215000832.never.591-kees@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Convert struct fealist away from 1-element array | expand |
merged into cifs-2.6.git for-next On Tue, Feb 14, 2023 at 6:16 PM Kees Cook <keescook@chromium.org> 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@samba.org> > Cc: Paulo Alcantara <pc@cjr.nz> > Cc: Ronnie Sahlberg <lsahlber@redhat.com> > Cc: Shyam Prasad N <sprasad@microsoft.com> > Cc: Tom Talpey <tom@talpey.com> > Cc: linux-cifs@vger.kernel.org > Cc: samba-technical@lists.samba.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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); > + 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 >
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@samba.org> > Cc: Paulo Alcantara <pc@cjr.nz> > Cc: Ronnie Sahlberg <lsahlber@redhat.com> > Cc: Shyam Prasad N <sprasad@microsoft.com> > Cc: Tom Talpey <tom@talpey.com> > Cc: linux-cifs@vger.kernel.org > Cc: samba-technical@lists.samba.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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 >
On Sat, Feb 10, 2024 at 01:13:06AM +0300, Vitaly Chikunov wrote: > 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@samba.org> > > Cc: Paulo Alcantara <pc@cjr.nz> > > Cc: Ronnie Sahlberg <lsahlber@redhat.com> > > Cc: Shyam Prasad N <sprasad@microsoft.com> > > Cc: Tom Talpey <tom@talpey.com> > > Cc: linux-cifs@vger.kernel.org > > Cc: samba-technical@lists.samba.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > 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 Yes, this seems likely. I would backport this change. > 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: 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: __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: __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: asm_int80_emulation+0x16/0x20 This appears to be a compat call? -Kees > > 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 > >
Kees, On Fri, Feb 09, 2024 at 04:02:32PM -0800, Kees Cook wrote: > On Sat, Feb 10, 2024 at 01:13:06AM +0300, Vitaly Chikunov wrote: > > 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@samba.org> > > > Cc: Paulo Alcantara <pc@cjr.nz> > > > Cc: Ronnie Sahlberg <lsahlber@redhat.com> > > > Cc: Shyam Prasad N <sprasad@microsoft.com> > > > Cc: Tom Talpey <tom@talpey.com> > > > Cc: linux-cifs@vger.kernel.org > > > Cc: samba-technical@lists.samba.org > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > 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 > > Yes, this seems likely. I would backport this change. > > > 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: 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: __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: __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: asm_int80_emulation+0x16/0x20 > > This appears to be a compat call? Likely. This is caused through Wine. Thanks, > > -Kees > > > > > 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 > > > > > -- > Kees Cook
Greg, Sasha, Can you please backport this commit (below) to a stable 6.1.y tree, it's confirmed be Kees this could cause kernel panic due to false positive strncpy fortify, and this is already happened for some users. Thanks, On Fri, Feb 09, 2024 at 04:02:32PM -0800, Kees Cook wrote: > On Sat, Feb 10, 2024 at 01:13:06AM +0300, Vitaly Chikunov wrote: > > > > 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@samba.org> > > > Cc: Paulo Alcantara <pc@cjr.nz> > > > Cc: Ronnie Sahlberg <lsahlber@redhat.com> > > > Cc: Shyam Prasad N <sprasad@microsoft.com> > > > Cc: Tom Talpey <tom@talpey.com> > > > Cc: linux-cifs@vger.kernel.org > > > Cc: samba-technical@lists.samba.org > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > 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 > > Yes, this seems likely. I would backport this change. > > > 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: 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: __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: __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: asm_int80_emulation+0x16/0x20 > > This appears to be a compat call? > > -Kees > > > > > 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 > > > > > -- > Kees Cook
On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > Greg, Sasha, > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > confirmed be Kees this could cause kernel panic due to false positive > strncpy fortify, and this is already happened for some users. What is the git commit id? thanks, greg k-h
Greg, On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > confirmed be Kees this could cause kernel panic due to false positive > > strncpy fortify, and this is already happened for some users. > > What is the git commit id? 398d5843c03261a2b68730f2f00643826bcec6ba Thanks, > > thanks, > > greg k-h
Greg, Sasha, On Sat, Feb 10, 2024 at 01:21:45PM +0300, Vitaly Chikunov wrote: > On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > > confirmed be Kees this could cause kernel panic due to false positive > > > strncpy fortify, and this is already happened for some users. > > > > What is the git commit id? > > 398d5843c03261a2b68730f2f00643826bcec6ba Can you please apply this to the next 6.1.y release? There is still non-theoretical crash as reported in https://lore.kernel.org/all/qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf/ If commit hash was not enough: commit 398d5843c03261a2b68730f2f00643826bcec6ba Author: Kees Cook <keescook@chromium.org> AuthorDate: Tue Feb 14 16:08:39 2023 -0800 cifs: Convert struct fealist away from 1-element array The commit is in mainline and is applying well to linux-6.1.y: (linux-6.1.y)$ git cherry-pick 398d5843c03261a2b68730f2f00643826bcec6ba Auto-merging fs/smb/client/cifspdu.h Auto-merging fs/smb/client/cifssmb.c [linux-6.1.y 4a80b516f202] cifs: Convert struct fealist away from 1-element array Author: Kees Cook <keescook@chromium.org> Date: Tue Feb 14 16:08:39 2023 -0800 2 files changed, 10 insertions(+), 10 deletions(-) Thanks, > > Thanks, > > > > > thanks, > > > > greg k-h
On Sun, Feb 18, 2024 at 12:50:16AM +0300, Vitaly Chikunov wrote: > Greg, Sasha, > > On Sat, Feb 10, 2024 at 01:21:45PM +0300, Vitaly Chikunov wrote: > > On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > > > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > > > confirmed be Kees this could cause kernel panic due to false positive > > > > strncpy fortify, and this is already happened for some users. > > > > > > What is the git commit id? > > > > 398d5843c03261a2b68730f2f00643826bcec6ba > > Can you please apply this to the next 6.1.y release? > > There is still non-theoretical crash as reported in > https://lore.kernel.org/all/qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf/ > > If commit hash was not enough: > > commit 398d5843c03261a2b68730f2f00643826bcec6ba > Author: Kees Cook <keescook@chromium.org> > AuthorDate: Tue Feb 14 16:08:39 2023 -0800 > > cifs: Convert struct fealist away from 1-element array > > The commit is in mainline and is applying well to linux-6.1.y: > > (linux-6.1.y)$ git cherry-pick 398d5843c03261a2b68730f2f00643826bcec6ba > Auto-merging fs/smb/client/cifspdu.h > Auto-merging fs/smb/client/cifssmb.c > [linux-6.1.y 4a80b516f202] cifs: Convert struct fealist away from 1-element array > Author: Kees Cook <keescook@chromium.org> > Date: Tue Feb 14 16:08:39 2023 -0800 > 2 files changed, 10 insertions(+), 10 deletions(-) It does not apply cleanly due to renames, can you provide a backported, and tested, patch please? thanks, greg k-h
Greg, On Sun, Feb 18, 2024 at 10:31:29AM +0100, Greg Kroah-Hartman wrote: > On Sun, Feb 18, 2024 at 12:50:16AM +0300, Vitaly Chikunov wrote: > > > > On Sat, Feb 10, 2024 at 01:21:45PM +0300, Vitaly Chikunov wrote: > > > On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > > > > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > > > > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > > > > confirmed be Kees this could cause kernel panic due to false positive > > > > > strncpy fortify, and this is already happened for some users. > > > > > > > > What is the git commit id? > > > > > > 398d5843c03261a2b68730f2f00643826bcec6ba > > > > Can you please apply this to the next 6.1.y release? > > > > There is still non-theoretical crash as reported in > > https://lore.kernel.org/all/qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf/ > > > > If commit hash was not enough: > > > > commit 398d5843c03261a2b68730f2f00643826bcec6ba > > Author: Kees Cook <keescook@chromium.org> > > AuthorDate: Tue Feb 14 16:08:39 2023 -0800 > > > > cifs: Convert struct fealist away from 1-element array > > > > The commit is in mainline and is applying well to linux-6.1.y: > > > > (linux-6.1.y)$ git cherry-pick 398d5843c03261a2b68730f2f00643826bcec6ba > > Auto-merging fs/smb/client/cifspdu.h > > Auto-merging fs/smb/client/cifssmb.c > > [linux-6.1.y 4a80b516f202] cifs: Convert struct fealist away from 1-element array > > Author: Kees Cook <keescook@chromium.org> > > Date: Tue Feb 14 16:08:39 2023 -0800 > > 2 files changed, 10 insertions(+), 10 deletions(-) > > It does not apply cleanly due to renames, can you provide a backported, > and tested, patch please? I cannot test it solves the bug since I don't use software that triggers the crash. But crash logic is obvious - sizeof of first element of char array is 1 byte and fortify code for strncpy issues panic. The patch is obviously missed. I can send that patch that is result of my git applying cleanly 398d5843c03261a2b68730f2f00643826bcec6ba. And I will try to build kernel and ensure it compiles well. Will this be enough? Thanks, > > thanks, > > greg k-h
Greg, On Sun, Feb 18, 2024 at 10:31:29AM +0100, Greg Kroah-Hartman wrote: > On Sun, Feb 18, 2024 at 12:50:16AM +0300, Vitaly Chikunov wrote: > > > > On Sat, Feb 10, 2024 at 01:21:45PM +0300, Vitaly Chikunov wrote: > > > On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > > > > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > > > > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > > > > confirmed be Kees this could cause kernel panic due to false positive > > > > > strncpy fortify, and this is already happened for some users. > > > > > > > > What is the git commit id? > > > > > > 398d5843c03261a2b68730f2f00643826bcec6ba > > > > Can you please apply this to the next 6.1.y release? > > > > There is still non-theoretical crash as reported in > > https://lore.kernel.org/all/qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf/ > > > > If commit hash was not enough: > > > > commit 398d5843c03261a2b68730f2f00643826bcec6ba > > Author: Kees Cook <keescook@chromium.org> > > AuthorDate: Tue Feb 14 16:08:39 2023 -0800 > > > > cifs: Convert struct fealist away from 1-element array > > > > The commit is in mainline and is applying well to linux-6.1.y: > > > > (linux-6.1.y)$ git cherry-pick 398d5843c03261a2b68730f2f00643826bcec6ba > > Auto-merging fs/smb/client/cifspdu.h > > Auto-merging fs/smb/client/cifssmb.c > > [linux-6.1.y 4a80b516f202] cifs: Convert struct fealist away from 1-element array > > Author: Kees Cook <keescook@chromium.org> > > Date: Tue Feb 14 16:08:39 2023 -0800 > > 2 files changed, 10 insertions(+), 10 deletions(-) > > It does not apply cleanly due to renames, can you provide a backported, > and tested, patch please? I sent the backported patch as you suggested[1] but I don't see it appearing in 6.1.79-rc1 review. Did I make some mistake while sending it? Thanks, [1] https://lore.kernel.org/stable/20240218111538.2592901-1-vt@altlinux.org/ > > thanks, > > greg k-h
Greg, On Wed, Feb 21, 2024 at 08:13:46PM +0300, Vitaly Chikunov wrote: > On Sun, Feb 18, 2024 at 10:31:29AM +0100, Greg Kroah-Hartman wrote: > > On Sun, Feb 18, 2024 at 12:50:16AM +0300, Vitaly Chikunov wrote: > > > > > > On Sat, Feb 10, 2024 at 01:21:45PM +0300, Vitaly Chikunov wrote: > > > > On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > > > > > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > > > > > > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > > > > > confirmed be Kees this could cause kernel panic due to false positive > > > > > > strncpy fortify, and this is already happened for some users. > > > > > > > > > > What is the git commit id? > > > > > > > > 398d5843c03261a2b68730f2f00643826bcec6ba > > > > > > Can you please apply this to the next 6.1.y release? > > > > > > There is still non-theoretical crash as reported in > > > https://lore.kernel.org/all/qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf/ > > > > > > If commit hash was not enough: > > > > > > commit 398d5843c03261a2b68730f2f00643826bcec6ba > > > Author: Kees Cook <keescook@chromium.org> > > > AuthorDate: Tue Feb 14 16:08:39 2023 -0800 > > > > > > cifs: Convert struct fealist away from 1-element array > > > > > > The commit is in mainline and is applying well to linux-6.1.y: > > > > > > (linux-6.1.y)$ git cherry-pick 398d5843c03261a2b68730f2f00643826bcec6ba > > > Auto-merging fs/smb/client/cifspdu.h > > > Auto-merging fs/smb/client/cifssmb.c > > > [linux-6.1.y 4a80b516f202] cifs: Convert struct fealist away from 1-element array > > > Author: Kees Cook <keescook@chromium.org> > > > Date: Tue Feb 14 16:08:39 2023 -0800 > > > 2 files changed, 10 insertions(+), 10 deletions(-) > > > > It does not apply cleanly due to renames, can you provide a backported, > > and tested, patch please? > > I sent the backported patch as you suggested[1] but I don't see it > appearing in 6.1.79-rc1 review. Did I make some mistake while sending > it? Testing update - user (who had the kernel panic) tested and reported to me that 6.1.78 with the patch applied does not cause the kernel panic anymore in their workflow. Thanks, > > Thanks, > > [1] https://lore.kernel.org/stable/20240218111538.2592901-1-vt@altlinux.org/ > > > > > thanks, > > > > greg k-h
Greg, Sasha, Ping. On Sun, Feb 18, 2024 at 10:31:29AM +0100, Greg Kroah-Hartman wrote: > On Sun, Feb 18, 2024 at 12:50:16AM +0300, Vitaly Chikunov wrote: > > On Sat, Feb 10, 2024 at 01:21:45PM +0300, Vitaly Chikunov wrote: > > > On Sat, Feb 10, 2024 at 10:19:46AM +0000, Greg Kroah-Hartman wrote: > > > > On Sat, Feb 10, 2024 at 03:33:14AM +0300, Vitaly Chikunov wrote: > > > > > > > > > > Can you please backport this commit (below) to a stable 6.1.y tree, it's > > > > > confirmed be Kees this could cause kernel panic due to false positive > > > > > strncpy fortify, and this is already happened for some users. > > > > > > > > What is the git commit id? > > > > > > 398d5843c03261a2b68730f2f00643826bcec6ba > > > > Can you please apply this to the next 6.1.y release? > > > > There is still non-theoretical crash as reported in > > https://lore.kernel.org/all/qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf/ > > > > If commit hash was not enough: > > > > commit 398d5843c03261a2b68730f2f00643826bcec6ba > > Author: Kees Cook <keescook@chromium.org> > > AuthorDate: Tue Feb 14 16:08:39 2023 -0800 > > > > cifs: Convert struct fealist away from 1-element array > > > > The commit is in mainline and is applying well to linux-6.1.y: > > > > (linux-6.1.y)$ git cherry-pick 398d5843c03261a2b68730f2f00643826bcec6ba > > Auto-merging fs/smb/client/cifspdu.h > > Auto-merging fs/smb/client/cifssmb.c > > [linux-6.1.y 4a80b516f202] cifs: Convert struct fealist away from 1-element array > > Author: Kees Cook <keescook@chromium.org> > > Date: Tue Feb 14 16:08:39 2023 -0800 > > 2 files changed, 10 insertions(+), 10 deletions(-) > > It does not apply cleanly due to renames, can you provide a backported, > and tested, patch please? Can you explain please why the patch submission [1] is silently not accepted so I could possibly resubmit it? [1] https://lore.kernel.org/stable/20240218111538.2592901-1-vt@altlinux.org/ It's tested to compile well and to fix the real crash. I also think the submission conforms to Option 3 of process/stable-kernel-rules.rst so I would be glad to know why if it isn't. Thanks, > > thanks, > > greg k-h
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); + 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;
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@samba.org> Cc: Paulo Alcantara <pc@cjr.nz> Cc: Ronnie Sahlberg <lsahlber@redhat.com> Cc: Shyam Prasad N <sprasad@microsoft.com> Cc: Tom Talpey <tom@talpey.com> Cc: linux-cifs@vger.kernel.org Cc: samba-technical@lists.samba.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/cifs/cifspdu.h | 4 ++-- fs/cifs/cifssmb.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-)