| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <qjyfz2xftsbch6aozgplxyjfyqnuhn7j44udrucls4pqa5ey35@adxvvrdtagqf>
Date: Sat, 10 Feb 2024 01:13:06 +0300
From: Vitaly Chikunov <vt@...linux.org>
To: Kees Cook <keescook@...omium.org>
Cc: Steve French <sfrench@...ba.org>, Paulo Alcantara <pc@....nz>,
Ronnie Sahlberg <lsahlber@...hat.com>, Shyam Prasad N <sprasad@...rosoft.com>,
Tom Talpey <tom@...pey.com>, linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] cifs: Convert struct fealist away from 1-element array
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@...ba.org>
> Cc: Paulo Alcantara <pc@....nz>
> Cc: Ronnie Sahlberg <lsahlber@...hat.com>
> Cc: Shyam Prasad N <sprasad@...rosoft.com>
> Cc: Tom Talpey <tom@...pey.com>
> Cc: linux-cifs@...r.kernel.org
> Cc: samba-technical@...ts.samba.org
> Signed-off-by: Kees Cook <keescook@...omium.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
>
Powered by blists - more mailing lists