| 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: <CAH2r5msf4yDYNd4Vwh7jNB3LNNje44JC8p5QnB5W2EpunepU5A@mail.gmail.com>
Date: Thu, 16 Feb 2023 23:37:34 -0600
From: Steve French <smfrench@...il.com>
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>,
linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
Tom Talpey <tom@...pey.com>, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH] cifs: Replace remaining 1-element arrays
merged into cifs-2.6.git pending testing
On Tue, Feb 14, 2023 at 6:17 PM Kees Cook <keescook@...omium.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].
>
> Replace the trailing 1-element array with a flexible array in the
> following structures:
>
> struct cifs_spnego_msg
> struct cifs_quota_data
> struct get_dfs_referral_rsp
> struct file_alt_name_info
> NEGOTIATE_RSP
> SESSION_SETUP_ANDX
> TCONX_REQ
> TCONX_RSP
> TCONX_RSP_EXT
> ECHO_REQ
> ECHO_RSP
> OPEN_REQ
> OPENX_REQ
> LOCK_REQ
> RENAME_REQ
> COPY_REQ
> COPY_RSP
> NT_RENAME_REQ
> DELETE_FILE_REQ
> DELETE_DIRECTORY_REQ
> CREATE_DIRECTORY_REQ
> QUERY_INFORMATION_REQ
> SETATTR_REQ
> TRANSACT_IOCTL_REQ
> TRANSACT_CHANGE_NOTIFY_REQ
> TRANSACTION2_QPI_REQ
> TRANSACTION2_SPI_REQ
> TRANSACTION2_FFIRST_REQ
> TRANSACTION2_GET_DFS_REFER_REQ
> FILE_UNIX_LINK_INFO
> FILE_DIRECTORY_INFO
> FILE_FULL_DIRECTORY_INFO
> SEARCH_ID_FULL_DIR_INFO
> FILE_BOTH_DIRECTORY_INFO
> FIND_FILE_STANDARD_INFO
>
> Replace the trailing 1-element array with a flexible array, but leave
> the existing structure padding:
>
> FILE_ALL_INFO
> FILE_UNIX_INFO
>
> Remove unused structures:
>
> struct gea
> struct gealist
>
> Adjust all related size calculations to match the changes to sizeof().
>
> 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: linux-cifs@...r.kernel.org
> Cc: samba-technical@...ts.samba.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> fs/cifs/cifs_spnego.h | 2 +-
> fs/cifs/cifspdu.h | 94 +++++++++++++++++++++----------------------
> fs/cifs/readdir.c | 6 +--
> fs/cifs/smb2pdu.c | 4 +-
> fs/cifs/smb2pdu.h | 2 +-
> 5 files changed, 52 insertions(+), 56 deletions(-)
>
> diff --git a/fs/cifs/cifs_spnego.h b/fs/cifs/cifs_spnego.h
> index 7f102ffeb675..e4d751b0c812 100644
> --- a/fs/cifs/cifs_spnego.h
> +++ b/fs/cifs/cifs_spnego.h
> @@ -24,7 +24,7 @@ struct cifs_spnego_msg {
> uint32_t flags;
> uint32_t sesskey_len;
> uint32_t secblob_len;
> - uint8_t data[1];
> + uint8_t data[];
> };
>
> #ifdef __KERNEL__
> diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
> index add73be4902c..445e3eaebcc1 100644
> --- a/fs/cifs/cifspdu.h
> +++ b/fs/cifs/cifspdu.h
> @@ -562,7 +562,7 @@ typedef union smb_com_session_setup_andx {
> __u32 Reserved;
> __le32 Capabilities; /* see below */
> __le16 ByteCount;
> - unsigned char SecurityBlob[1]; /* followed by */
> + unsigned char SecurityBlob[]; /* followed by */
> /* STRING NativeOS */
> /* STRING NativeLanMan */
> } __attribute__((packed)) req; /* NTLM request format (with
> @@ -582,7 +582,7 @@ typedef union smb_com_session_setup_andx {
> __u32 Reserved; /* see below */
> __le32 Capabilities;
> __le16 ByteCount;
> - unsigned char CaseInsensitivePassword[1]; /* followed by: */
> + unsigned char CaseInsensitivePassword[]; /* followed by: */
> /* unsigned char * CaseSensitivePassword; */
> /* STRING AccountName */
> /* STRING PrimaryDomain */
> @@ -599,7 +599,7 @@ typedef union smb_com_session_setup_andx {
> __le16 Action; /* see below */
> __le16 SecurityBlobLength;
> __u16 ByteCount;
> - unsigned char SecurityBlob[1]; /* followed by */
> + unsigned char SecurityBlob[]; /* followed by */
> /* unsigned char * NativeOS; */
> /* unsigned char * NativeLanMan; */
> /* unsigned char * PrimaryDomain; */
> @@ -618,7 +618,7 @@ typedef union smb_com_session_setup_andx {
> __le16 PasswordLength;
> __u32 Reserved; /* encrypt key len and offset */
> __le16 ByteCount;
> - unsigned char AccountPassword[1]; /* followed by */
> + unsigned char AccountPassword[]; /* followed by */
> /* STRING AccountName */
> /* STRING PrimaryDomain */
> /* STRING NativeOS */
> @@ -632,7 +632,7 @@ typedef union smb_com_session_setup_andx {
> __le16 AndXOffset;
> __le16 Action; /* see below */
> __u16 ByteCount;
> - unsigned char NativeOS[1]; /* followed by */
> + unsigned char NativeOS[]; /* followed by */
> /* unsigned char * NativeLanMan; */
> /* unsigned char * PrimaryDomain; */
> } __attribute__((packed)) old_resp; /* pre-NTLM (LANMAN2.1) response */
> @@ -693,7 +693,7 @@ typedef struct smb_com_tconx_req {
> __le16 Flags; /* see below */
> __le16 PasswordLength;
> __le16 ByteCount;
> - unsigned char Password[1]; /* followed by */
> + unsigned char Password[]; /* followed by */
> /* STRING Path *//* \\server\share name */
> /* STRING Service */
> } __attribute__((packed)) TCONX_REQ;
> @@ -705,7 +705,7 @@ typedef struct smb_com_tconx_rsp {
> __le16 AndXOffset;
> __le16 OptionalSupport; /* see below */
> __u16 ByteCount;
> - unsigned char Service[1]; /* always ASCII, not Unicode */
> + unsigned char Service[]; /* always ASCII, not Unicode */
> /* STRING NativeFileSystem */
> } __attribute__((packed)) TCONX_RSP;
>
> @@ -718,7 +718,7 @@ typedef struct smb_com_tconx_rsp_ext {
> __le32 MaximalShareAccessRights;
> __le32 GuestMaximalShareAccessRights;
> __u16 ByteCount;
> - unsigned char Service[1]; /* always ASCII, not Unicode */
> + unsigned char Service[]; /* always ASCII, not Unicode */
> /* STRING NativeFileSystem */
> } __attribute__((packed)) TCONX_RSP_EXT;
>
> @@ -755,14 +755,14 @@ typedef struct smb_com_echo_req {
> struct smb_hdr hdr;
> __le16 EchoCount;
> __le16 ByteCount;
> - char Data[1];
> + char Data[];
> } __attribute__((packed)) ECHO_REQ;
>
> typedef struct smb_com_echo_rsp {
> struct smb_hdr hdr;
> __le16 SequenceNumber;
> __le16 ByteCount;
> - char Data[1];
> + char Data[];
> } __attribute__((packed)) ECHO_RSP;
>
> typedef struct smb_com_logoff_andx_req {
> @@ -862,7 +862,7 @@ typedef struct smb_com_open_req { /* also handles create */
> __le32 ImpersonationLevel;
> __u8 SecurityFlags;
> __le16 ByteCount;
> - char fileName[1];
> + char fileName[];
> } __attribute__((packed)) OPEN_REQ;
>
> /* open response: oplock levels */
> @@ -937,7 +937,7 @@ typedef struct smb_com_openx_req {
> __le32 Timeout;
> __le32 Reserved;
> __le16 ByteCount; /* file name follows */
> - char fileName[1];
> + char fileName[];
> } __attribute__((packed)) OPENX_REQ;
>
> typedef struct smb_com_openx_rsp {
> @@ -1085,7 +1085,7 @@ typedef struct smb_com_lock_req {
> __le16 NumberOfUnlocks;
> __le16 NumberOfLocks;
> __le16 ByteCount;
> - LOCKING_ANDX_RANGE Locks[1];
> + LOCKING_ANDX_RANGE Locks[];
> } __attribute__((packed)) LOCK_REQ;
>
> /* lock type */
> @@ -1114,7 +1114,7 @@ typedef struct smb_com_rename_req {
> __le16 SearchAttributes; /* target file attributes */
> __le16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII or Unicode */
> - unsigned char OldFileName[1];
> + unsigned char OldFileName[];
> /* followed by __u8 BufferFormat2 */
> /* followed by NewFileName */
> } __attribute__((packed)) RENAME_REQ;
> @@ -1134,7 +1134,7 @@ typedef struct smb_com_copy_req {
> __le16 Flags;
> __le16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII or Unicode */
> - unsigned char OldFileName[1];
> + unsigned char OldFileName[];
> /* followed by __u8 BufferFormat2 */
> /* followed by NewFileName string */
> } __attribute__((packed)) COPY_REQ;
> @@ -1144,7 +1144,7 @@ typedef struct smb_com_copy_rsp {
> __le16 CopyCount; /* number of files copied */
> __u16 ByteCount; /* may be zero */
> __u8 BufferFormat; /* 0x04 - only present if errored file follows */
> - unsigned char ErrorFileName[1]; /* only present if error in copy */
> + unsigned char ErrorFileName[]; /* only present if error in copy */
> } __attribute__((packed)) COPY_RSP;
>
> #define CREATE_HARD_LINK 0x103
> @@ -1158,7 +1158,7 @@ typedef struct smb_com_nt_rename_req { /* A5 - also used for create hardlink */
> __le32 ClusterCount;
> __le16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII or Unicode */
> - unsigned char OldFileName[1];
> + unsigned char OldFileName[];
> /* followed by __u8 BufferFormat2 */
> /* followed by NewFileName */
> } __attribute__((packed)) NT_RENAME_REQ;
> @@ -1173,7 +1173,7 @@ typedef struct smb_com_delete_file_req {
> __le16 SearchAttributes;
> __le16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII */
> - unsigned char fileName[1];
> + unsigned char fileName[];
> } __attribute__((packed)) DELETE_FILE_REQ;
>
> typedef struct smb_com_delete_file_rsp {
> @@ -1185,7 +1185,7 @@ typedef struct smb_com_delete_directory_req {
> struct smb_hdr hdr; /* wct = 0 */
> __le16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII */
> - unsigned char DirName[1];
> + unsigned char DirName[];
> } __attribute__((packed)) DELETE_DIRECTORY_REQ;
>
> typedef struct smb_com_delete_directory_rsp {
> @@ -1197,7 +1197,7 @@ typedef struct smb_com_create_directory_req {
> struct smb_hdr hdr; /* wct = 0 */
> __le16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII */
> - unsigned char DirName[1];
> + unsigned char DirName[];
> } __attribute__((packed)) CREATE_DIRECTORY_REQ;
>
> typedef struct smb_com_create_directory_rsp {
> @@ -1209,7 +1209,7 @@ typedef struct smb_com_query_information_req {
> struct smb_hdr hdr; /* wct = 0 */
> __le16 ByteCount; /* 1 + namelen + 1 */
> __u8 BufferFormat; /* 4 = ASCII */
> - unsigned char FileName[1];
> + unsigned char FileName[];
> } __attribute__((packed)) QUERY_INFORMATION_REQ;
>
> typedef struct smb_com_query_information_rsp {
> @@ -1229,7 +1229,7 @@ typedef struct smb_com_setattr_req {
> __le16 reserved[5]; /* must be zero */
> __u16 ByteCount;
> __u8 BufferFormat; /* 4 = ASCII */
> - unsigned char fileName[1];
> + unsigned char fileName[];
> } __attribute__((packed)) SETATTR_REQ;
>
> typedef struct smb_com_setattr_rsp {
> @@ -1311,7 +1311,7 @@ typedef struct smb_com_transaction_ioctl_req {
> __u8 IsRootFlag; /* 1 = apply command to root of share (must be DFS) */
> __le16 ByteCount;
> __u8 Pad[3];
> - __u8 Data[1];
> + __u8 Data[];
> } __attribute__((packed)) TRANSACT_IOCTL_REQ;
>
> typedef struct smb_com_transaction_compr_ioctl_req {
> @@ -1430,7 +1430,7 @@ typedef struct smb_com_transaction_change_notify_req {
> __u8 Reserved2;
> __le16 ByteCount;
> /* __u8 Pad[3];*/
> -/* __u8 Data[1];*/
> +/* __u8 Data[];*/
> } __attribute__((packed)) TRANSACT_CHANGE_NOTIFY_REQ;
>
> /* BB eventually change to use generic ntransact rsp struct
> @@ -1519,7 +1519,7 @@ struct cifs_quota_data {
> __u64 space_used;
> __u64 soft_limit;
> __u64 hard_limit;
> - char sid[1]; /* variable size? */
> + char sid[]; /* variable size? */
> } __attribute__((packed));
>
> /* quota sub commands */
> @@ -1671,7 +1671,7 @@ typedef struct smb_com_transaction2_qpi_req {
> __u8 Pad;
> __le16 InformationLevel;
> __u32 Reserved4;
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) TRANSACTION2_QPI_REQ;
>
> typedef struct smb_com_transaction2_qpi_rsp {
> @@ -1704,7 +1704,7 @@ typedef struct smb_com_transaction2_spi_req {
> __u16 Pad1;
> __le16 InformationLevel;
> __u32 Reserved4;
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) TRANSACTION2_SPI_REQ;
>
> typedef struct smb_com_transaction2_spi_rsp {
> @@ -1809,7 +1809,7 @@ typedef struct smb_com_transaction2_ffirst_req {
> __le16 SearchFlags;
> __le16 InformationLevel;
> __le32 SearchStorageType;
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) TRANSACTION2_FFIRST_REQ;
>
> typedef struct smb_com_transaction2_ffirst_rsp {
> @@ -2020,7 +2020,7 @@ typedef struct smb_com_transaction2_get_dfs_refer_req {
> perhaps?) followed by one byte pad - doesn't
> seem to matter though */
> __le16 MaxReferralLevel;
> - char RequestFileName[1];
> + char RequestFileName[];
> } __attribute__((packed)) TRANSACTION2_GET_DFS_REFER_REQ;
>
> #define DFS_VERSION cpu_to_le16(0x0003)
> @@ -2049,7 +2049,7 @@ struct get_dfs_referral_rsp {
> __le16 PathConsumed;
> __le16 NumberOfReferrals;
> __le32 DFSFlags;
> - REFERRAL3 referrals[1]; /* array of level 3 dfs_referral structures */
> + REFERRAL3 referrals[]; /* array of level 3 dfs_referral structures */
> /* followed by the strings pointed to by the referral structures */
> } __packed;
>
> @@ -2284,7 +2284,10 @@ typedef struct { /* data block encoding of response to level 263 QPathInfo */
> __le32 Mode;
> __le32 AlignmentRequirement;
> __le32 FileNameLength;
> - char FileName[1];
> + union {
> + char __pad;
> + DECLARE_FLEX_ARRAY(char, FileName);
> + };
> } __attribute__((packed)) FILE_ALL_INFO; /* level 0x107 QPathInfo */
>
> typedef struct {
> @@ -2322,7 +2325,7 @@ typedef struct {
> } __attribute__((packed)) FILE_UNIX_BASIC_INFO; /* level 0x200 QPathInfo */
>
> typedef struct {
> - char LinkDest[1];
> + DECLARE_FLEX_ARRAY(char, LinkDest);
> } __attribute__((packed)) FILE_UNIX_LINK_INFO; /* level 0x201 QPathInfo */
>
> /* The following three structures are needed only for
> @@ -2371,7 +2374,7 @@ struct file_end_of_file_info {
> } __attribute__((packed)); /* size info, level 0x104 for set, 0x106 for query */
>
> struct file_alt_name_info {
> - __u8 alt_name[1];
> + DECLARE_FLEX_ARRAY(__u8, alt_name);
> } __attribute__((packed)); /* level 0x0108 */
>
> struct file_stream_info {
> @@ -2480,7 +2483,10 @@ typedef struct {
> __le32 NextEntryOffset;
> __u32 ResumeKey; /* as with FileIndex - no need to convert */
> FILE_UNIX_BASIC_INFO basic;
> - char FileName[1];
> + union {
> + char __pad;
> + DECLARE_FLEX_ARRAY(char, FileName);
> + };
> } __attribute__((packed)) FILE_UNIX_INFO; /* level 0x202 */
>
> typedef struct {
> @@ -2494,7 +2500,7 @@ typedef struct {
> __le64 AllocationSize;
> __le32 ExtFileAttributes;
> __le32 FileNameLength;
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) FILE_DIRECTORY_INFO; /* level 0x101 FF resp data */
>
> typedef struct {
> @@ -2509,7 +2515,7 @@ typedef struct {
> __le32 ExtFileAttributes;
> __le32 FileNameLength;
> __le32 EaSize; /* length of the xattrs */
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) FILE_FULL_DIRECTORY_INFO; /* level 0x102 rsp data */
>
> typedef struct {
> @@ -2526,7 +2532,7 @@ typedef struct {
> __le32 EaSize; /* EA size */
> __le32 Reserved;
> __le64 UniqueId; /* inode num - le since Samba puts ino in low 32 bit*/
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) SEARCH_ID_FULL_DIR_INFO; /* level 0x105 FF rsp data */
>
> typedef struct {
> @@ -2544,7 +2550,7 @@ typedef struct {
> __u8 ShortNameLength;
> __u8 Reserved;
> __u8 ShortName[24];
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) FILE_BOTH_DIRECTORY_INFO; /* level 0x104 FFrsp data */
>
> typedef struct {
> @@ -2559,7 +2565,7 @@ typedef struct {
> __le32 AllocationSize;
> __le16 Attributes; /* verify not u32 */
> __u8 FileNameLength;
> - char FileName[1];
> + char FileName[];
> } __attribute__((packed)) FIND_FILE_STANDARD_INFO; /* level 0x1 FF resp data */
>
>
> @@ -2569,16 +2575,6 @@ struct win_dev {
> __le64 minor;
> } __attribute__((packed));
>
> -struct gea {
> - unsigned char name_len;
> - char name[1];
> -} __attribute__((packed));
> -
> -struct gealist {
> - unsigned long list_len;
> - struct gea list[1];
> -} __attribute__((packed));
> -
> struct fea {
> unsigned char EA_flags;
> __u8 name_len;
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index 2d75ba5aaa8a..ef638086d734 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -495,7 +495,7 @@ static char *nxt_dir_entry(char *old_entry, char *end_of_smb, int level)
> FIND_FILE_STANDARD_INFO *pfData;
> pfData = (FIND_FILE_STANDARD_INFO *)pDirInfo;
>
> - new_entry = old_entry + sizeof(FIND_FILE_STANDARD_INFO) +
> + new_entry = old_entry + sizeof(FIND_FILE_STANDARD_INFO) + 1 +
> pfData->FileNameLength;
> } else {
> u32 next_offset = le32_to_cpu(pDirInfo->NextEntryOffset);
> @@ -513,9 +513,9 @@ static char *nxt_dir_entry(char *old_entry, char *end_of_smb, int level)
> new_entry, end_of_smb, old_entry);
> return NULL;
> } else if (((level == SMB_FIND_FILE_INFO_STANDARD) &&
> - (new_entry + sizeof(FIND_FILE_STANDARD_INFO) > end_of_smb))
> + (new_entry + sizeof(FIND_FILE_STANDARD_INFO) + 1 > end_of_smb))
> || ((level != SMB_FIND_FILE_INFO_STANDARD) &&
> - (new_entry + sizeof(FILE_DIRECTORY_INFO) > end_of_smb))) {
> + (new_entry + sizeof(FILE_DIRECTORY_INFO) + 1 > end_of_smb))) {
> cifs_dbg(VFS, "search entry %p extends after end of SMB %p\n",
> new_entry, end_of_smb);
> return NULL;
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index c304678514a5..4982f5e65e13 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -5010,10 +5010,10 @@ smb2_parse_query_directory(struct cifs_tcon *tcon,
>
> switch (srch_inf->info_level) {
> case SMB_FIND_FILE_DIRECTORY_INFO:
> - info_buf_size = sizeof(FILE_DIRECTORY_INFO) - 1;
> + info_buf_size = sizeof(FILE_DIRECTORY_INFO);
> break;
> case SMB_FIND_FILE_ID_FULL_DIR_INFO:
> - info_buf_size = sizeof(SEARCH_ID_FULL_DIR_INFO) - 1;
> + info_buf_size = sizeof(SEARCH_ID_FULL_DIR_INFO);
> break;
> case SMB_FIND_FILE_POSIX_INFO:
> /* note that posix payload are variable size */
> diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h
> index a5773a06aba8..2114e8a0c63a 100644
> --- a/fs/cifs/smb2pdu.h
> +++ b/fs/cifs/smb2pdu.h
> @@ -371,7 +371,7 @@ struct smb2_file_id_extd_directory_info {
> __le32 EaSize; /* EA size */
> __le32 ReparsePointTag; /* valid if FILE_ATTR_REPARSE_POINT set in FileAttributes */
> __le64 UniqueId; /* inode num - le since Samba puts ino in low 32 bit */
> - char FileName[1];
> + char FileName[];
> } __packed; /* level 60 */
>
> extern char smb2_padding[7];
> --
> 2.34.1
>
--
Thanks,
Steve
Powered by blists - more mailing lists