cifs: Convert struct fealist away from 1-element array

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 <[email protected]>

Cc Paulo Alcantara <[email protected]>

Cc Ronnie Sahlberg <[email protected]>

Cc Shyam Prasad N <[email protected]>

Cc Tom Talpey <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 fs/cifs/cifspdu.h |  4 ++--
 fs/cifs/cifssmb.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/cifs/cifspdu.h +2 -2

index 623caece2b10..add73be4902c 100644
@@ -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 */

fs/cifs/cifssmb.c +8 -9

index 60dd4e37030a..7c587157d030 100644
@@ -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

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 <[email protected]>

Cc Paulo Alcantara <[email protected]>

Cc Ronnie Sahlberg <[email protected]>

Cc Shyam Prasad N <[email protected]>

Cc Tom Talpey <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 fs/cifs/cifspdu.h |  4 ++--
 fs/cifs/cifssmb.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/cifs/cifspdu.h +2 -2

index 623caece2b10..add73be4902c 100644
@@ -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 */

fs/cifs/cifssmb.c +2 -3

index 60dd4e37030a..7c587157d030 100644
@@ -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;

fs/cifs/cifssmb.c +3 -1

+	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;

Signature · 1 line

2.34.1

Kees,

Quoted 82 lines · Kees Cook

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 <[email protected]>

Cc Paulo Alcantara <[email protected]>

Cc Ronnie Sahlberg <[email protected]>

Cc Shyam Prasad N <[email protected]>

Cc Tom Talpey <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 fs/cifs/cifspdu.h |  4 ++--
 fs/cifs/cifssmb.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/cifs/cifspdu.h +2 -2

index 623caece2b10..add73be4902c 100644
@@ -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 */

fs/cifs/cifssmb.c +2 -3

index 60dd4e37030a..7c587157d030 100644
@@ -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;

fs/cifs/cifssmb.c CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, · line 5934

Quoted 10 lines

fs/cifs/cifssmb.c +3 -4

+	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:  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

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.

Quoted 17 lines

fs/cifs/cifssmb.c +3 -1

+	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;

Signature · 1 line

2.34.1

Quoted 97 lines · Vitaly Chikunov

Quoted 82 lines · Kees Cook

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 <[email protected]>

Cc Paulo Alcantara <[email protected]>

Cc Ronnie Sahlberg <[email protected]>

Cc Shyam Prasad N <[email protected]>

Cc Tom Talpey <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 fs/cifs/cifspdu.h |  4 ++--
 fs/cifs/cifssmb.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/cifs/cifspdu.h +2 -2

index 623caece2b10..add73be4902c 100644
@@ -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 */

fs/cifs/cifssmb.c +2 -3

index 60dd4e37030a..7c587157d030 100644
@@ -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;

fs/cifs/cifssmb.c CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, · line 5934

Quoted 10 lines

fs/cifs/cifssmb.c +3 -4

+	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:

Quoted 1 line

   Jan 24 15:15:20 kalt2test.dpt.local kernel: detected buffer overflow in strncpy

Yes, this seems likely. I would backport this change.

Quoted 5 lines

   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]

Quoted 8 lines

   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

Quoted 40 lines

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.

Quoted 17 lines

fs/cifs/cifssmb.c +3 -1

+	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;

Signature · 1 line

2.34.1

Signature · 1 line

Kees Cook

Quoted 98 lines · Vitaly Chikunov

Quoted 82 lines · Kees Cook

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 <[email protected]>

Cc Paulo Alcantara <[email protected]>

Cc Ronnie Sahlberg <[email protected]>

Cc Shyam Prasad N <[email protected]>

Cc Tom Talpey <[email protected]>

Cc [email protected]

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 fs/cifs/cifspdu.h |  4 ++--
 fs/cifs/cifssmb.c | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

fs/cifs/cifspdu.h +2 -2

index 623caece2b10..add73be4902c 100644
@@ -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 */

fs/cifs/cifssmb.c +2 -3

index 60dd4e37030a..7c587157d030 100644
@@ -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;

fs/cifs/cifssmb.c CIFSSMBSetEA(const unsigned int xid, struct cifs_tcon *tcon, · line 5934

Quoted 10 lines

fs/cifs/cifssmb.c +3 -4

+	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:

Quoted 1 line

   Jan 24 15:15:20 kalt2test.dpt.local kernel: detected buffer overflow in strncpy

Yes, this seems likely. I would backport this change.

Quoted 5 lines

   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]

Quoted 8 lines

   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

Quoted 40 lines

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.

Quoted 17 lines

fs/cifs/cifssmb.c +3 -1

+	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;

Signature · 1 line

2.34.1

Signature · 1 line

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.

Quoted 4 lines · Vitaly Chikunov

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

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Vitaly Chikunov

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,

Quoted 4 lines

thanks,

greg k-h

Greg, Sasha,

Quoted 10 lines · Vitaly Chikunov

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Vitaly Chikunov

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 <[email protected]>
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 <[email protected]>
Date: Tue Feb 14 16:08:39 2023 -0800

   2 files changed, 10 insertions(+), 10 deletions(-)

Quoted 28 lines · Vitaly Chikunov

Quoted 10 lines · Vitaly Chikunov

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Vitaly Chikunov

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 <[email protected]>
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:

Quoted 7 lines

(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 <[email protected]>
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

Quoted 28 lines · Vitaly Chikunov

Quoted 10 lines · Vitaly Chikunov

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Vitaly Chikunov

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 <[email protected]>
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:

Quoted 7 lines

(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 <[email protected]>
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

Quoted 37 lines · Greg Kroah-Hartman

Quoted 35 lines · Vitaly Chikunov

Quoted 10 lines · Vitaly Chikunov

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Vitaly Chikunov

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 <[email protected]>
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 <[email protected]>
Date: Tue Feb 14 16:08:39 2023 -0800

   2 files changed, 10 insertions(+), 10 deletions(-)

Quoted 2 lines

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/[email protected]/

Quoted 4 lines

thanks,

greg k-h

Quoted 27 lines · Vitaly Chikunov

Quoted 10 lines · Vitaly Chikunov

Quoted 7 lines · Greg Kroah-Hartman

Quoted 4 lines · Vitaly Chikunov

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 <[email protected]>
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:

Quoted 7 lines

(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 <[email protected]>
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