Messages in this thread | | | Date | Wed, 24 Apr 2024 07:08:32 +1000 | Subject | Re: [PATCH][next] smb: smb2pdu.h: Avoid -Wflex-array-member-not-at-end warnings | From | "Gustavo A. R. Silva" <> |
| |
On 23/04/24 14:47, Gustavo A. R. Silva wrote: > > > On 23/04/24 14:15, Steve French wrote: >> This looks reasonably safe (running the usual regression tests on it now). >> >> Reminds me though that we have to be careful (e.g. the recent fix for >> regression caused by cleanup). > > mmh... it seems that the offending commit was never CC'd to the linux-hardening > list, hence it wasn't reviewed by us. > > After reviewing both, the offending commit and the fix, both seem to be wrong. > > for __packed structs, you should use __struct_group(): > > __struct_group(network_open_info, group_name, __packed, struct_members); > > The _packed in the commit 0268a7cc7fdc is not an attribute, it's the name > for the group. So, it's not actually doing what the submitter thinks it does.
Sorry about this, I'm still too sleep deprived, aghr... I confused the suffix _attr with _tag.
The fix is correct!
Thanks! -- Gustavo
> >> >> Thoughts about whether should be sent in rc6 or wait till 6.10? 51 >> warnings does sound >> distracting though so might be worth going in sooner rather than later. > > There is actually no hurry. :) > > Thanks > -- > Gustavo > >> >> commit 0268a7cc7fdc47d90b6c18859de7718d5059f6f1 >> Author: Namjae Jeon <linkinjeon@kernel.org> >> Date: Fri Apr 19 23:46:34 2024 +0900 >> >> ksmbd: common: use struct_group_attr instead of struct_group for >> network_open_info >> >> 4byte padding cause the connection issue with the applications of MacOS. >> smb2_close response size increases by 4 bytes by padding, And the smb >> client of MacOS check it and stop the connection. This patch use >> struct_group_attr instead of struct_group for network_open_info to use >> __packed to avoid padding. >> >> >> On Tue, Apr 23, 2024 at 1:58 PM Gustavo A. R. Silva >> <gustavo@embeddedor.com> wrote: >>> >>> Hi all, >>> >>> Friendly ping: who can take this, please? >>> >>> Thanks >>> -- >>> Gustavo >>> >>> On 11/04/24 09:35, Gustavo A. R. Silva wrote: >>>> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting >>>> ready to enable it globally. >>>> >>>> So, in order to avoid ending up with a flexible-array member in the >>>> middle of multiple other structs, we use the `__struct_group()` helper >>>> to separate the flexible array from the rest of the members in the >>>> flexible structure, and use the tagged `struct create_context_hdr` >>>> instead of `struct create_context`. >>>> >>>> So, with these changes, fix 51 of the following warnings[1]: >>>> >>>> fs/smb/client/../common/smb2pdu.h:1225:31: warning: structure containing a flexible array member is not at the end of another structure >>>> [-Wflex-array-member-not-at-end] >>>> >>>> Link: https://gist.github.com/GustavoARSilva/772526a39be3dd4db39e71497f0a9893 [1] >>>> Link: https://github.com/KSPP/linux/issues/202 >>>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >>>> --- >>>> fs/smb/client/smb2pdu.h | 12 ++++++------ >>>> fs/smb/common/smb2pdu.h | 33 ++++++++++++++++++--------------- >>>> fs/smb/server/smb2pdu.h | 18 +++++++++--------- >>>> 3 files changed, 33 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h >>>> index c72a3b2886b7..1a02bd9e0c00 100644 >>>> --- a/fs/smb/client/smb2pdu.h >>>> +++ b/fs/smb/client/smb2pdu.h >>>> @@ -145,7 +145,7 @@ struct durable_context_v2 { >>>> } __packed; >>>> >>>> struct create_durable_v2 { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct durable_context_v2 dcontext; >>>> } __packed; >>>> @@ -167,7 +167,7 @@ struct durable_reconnect_context_v2_rsp { >>>> } __packed; >>>> >>>> struct create_durable_handle_reconnect_v2 { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct durable_reconnect_context_v2 dcontext; >>>> __u8 Pad[4]; >>>> @@ -175,7 +175,7 @@ struct create_durable_handle_reconnect_v2 { >>>> >>>> /* See MS-SMB2 2.2.13.2.5 */ >>>> struct crt_twarp_ctxt { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le64 Timestamp; >>>> >>>> @@ -183,12 +183,12 @@ struct crt_twarp_ctxt { >>>> >>>> /* See MS-SMB2 2.2.13.2.9 */ >>>> struct crt_query_id_ctxt { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> } __packed; >>>> >>>> struct crt_sd_ctxt { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct smb3_sd sd; >>>> } __packed; >>>> @@ -415,7 +415,7 @@ struct smb2_posix_info_parsed { >>>> }; >>>> >>>> struct smb2_create_ea_ctx { >>>> - struct create_context ctx; >>>> + struct create_context_hdr ctx; >>>> __u8 name[8]; >>>> struct smb2_file_full_ea_info ea; >>>> } __packed; >>>> diff --git a/fs/smb/common/smb2pdu.h b/fs/smb/common/smb2pdu.h >>>> index 1b594307c9d5..eab9d49c63ba 100644 >>>> --- a/fs/smb/common/smb2pdu.h >>>> +++ b/fs/smb/common/smb2pdu.h >>>> @@ -1171,12 +1171,15 @@ struct smb2_server_client_notification { >>>> #define SMB2_CREATE_FLAG_REPARSEPOINT 0x01 >>>> >>>> struct create_context { >>>> - __le32 Next; >>>> - __le16 NameOffset; >>>> - __le16 NameLength; >>>> - __le16 Reserved; >>>> - __le16 DataOffset; >>>> - __le32 DataLength; >>>> + /* New members must be added within the struct_group() macro below. */ >>>> + __struct_group(create_context_hdr, hdr, __packed, >>>> + __le32 Next; >>>> + __le16 NameOffset; >>>> + __le16 NameLength; >>>> + __le16 Reserved; >>>> + __le16 DataOffset; >>>> + __le32 DataLength; >>>> + ); >>>> __u8 Buffer[]; >>>> } __packed; >>>> >>>> @@ -1222,7 +1225,7 @@ struct smb2_create_rsp { >>>> } __packed; >>>> >>>> struct create_posix { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[16]; >>>> __le32 Mode; >>>> __u32 Reserved; >>>> @@ -1230,7 +1233,7 @@ struct create_posix { >>>> >>>> /* See MS-SMB2 2.2.13.2.3 and MS-SMB2 2.2.13.2.4 */ >>>> struct create_durable { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> union { >>>> __u8 Reserved[16]; >>>> @@ -1243,14 +1246,14 @@ struct create_durable { >>>> >>>> /* See MS-SMB2 2.2.13.2.5 */ >>>> struct create_mxac_req { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le64 Timestamp; >>>> } __packed; >>>> >>>> /* See MS-SMB2 2.2.14.2.5 */ >>>> struct create_mxac_rsp { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le32 QueryStatus; >>>> __le32 MaximalAccess; >>>> @@ -1286,13 +1289,13 @@ struct lease_context_v2 { >>>> } __packed; >>>> >>>> struct create_lease { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct lease_context lcontext; >>>> } __packed; >>>> >>>> struct create_lease_v2 { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct lease_context_v2 lcontext; >>>> __u8 Pad[4]; >>>> @@ -1300,7 +1303,7 @@ struct create_lease_v2 { >>>> >>>> /* See MS-SMB2 2.2.14.2.9 */ >>>> struct create_disk_id_rsp { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le64 DiskFileId; >>>> __le64 VolumeId; >>>> @@ -1309,7 +1312,7 @@ struct create_disk_id_rsp { >>>> >>>> /* See MS-SMB2 2.2.13.2.13 */ >>>> struct create_app_inst_id { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[16]; >>>> __le32 StructureSize; /* Must be 20 */ >>>> __u16 Reserved; >>>> @@ -1318,7 +1321,7 @@ struct create_app_inst_id { >>>> >>>> /* See MS-SMB2 2.2.13.2.15 */ >>>> struct create_app_inst_id_vers { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[16]; >>>> __le32 StructureSize; /* Must be 24 */ >>>> __u16 Reserved; >>>> diff --git a/fs/smb/server/smb2pdu.h b/fs/smb/server/smb2pdu.h >>>> index bd1d2a0e9203..643f5e1cfe35 100644 >>>> --- a/fs/smb/server/smb2pdu.h >>>> +++ b/fs/smb/server/smb2pdu.h >>>> @@ -64,7 +64,7 @@ struct preauth_integrity_info { >>>> #define SMB2_SESSION_TIMEOUT (10 * HZ) >>>> >>>> struct create_durable_req_v2 { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le32 Timeout; >>>> __le32 Flags; >>>> @@ -73,7 +73,7 @@ struct create_durable_req_v2 { >>>> } __packed; >>>> >>>> struct create_durable_reconn_req { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> union { >>>> __u8 Reserved[16]; >>>> @@ -85,7 +85,7 @@ struct create_durable_reconn_req { >>>> } __packed; >>>> >>>> struct create_durable_reconn_v2_req { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct { >>>> __u64 PersistentFileId; >>>> @@ -96,13 +96,13 @@ struct create_durable_reconn_v2_req { >>>> } __packed; >>>> >>>> struct create_alloc_size_req { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le64 AllocationSize; >>>> } __packed; >>>> >>>> struct create_durable_rsp { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> union { >>>> __u8 Reserved[8]; >>>> @@ -114,7 +114,7 @@ struct create_durable_rsp { >>>> /* Flags */ >>>> #define SMB2_DHANDLE_FLAG_PERSISTENT 0x00000002 >>>> struct create_durable_v2_rsp { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> __le32 Timeout; >>>> __le32 Flags; >>>> @@ -122,7 +122,7 @@ struct create_durable_v2_rsp { >>>> >>>> /* equivalent of the contents of SMB3.1.1 POSIX open context response */ >>>> struct create_posix_rsp { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[16]; >>>> __le32 nlink; >>>> __le32 reparse_tag; >>>> @@ -381,13 +381,13 @@ struct smb2_ea_info { >>>> } __packed; /* level 15 Query */ >>>> >>>> struct create_ea_buf_req { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct smb2_ea_info ea; >>>> } __packed; >>>> >>>> struct create_sd_buf_req { >>>> - struct create_context ccontext; >>>> + struct create_context_hdr ccontext; >>>> __u8 Name[8]; >>>> struct smb_ntsd ntsd; >>>> } __packed; >>> >> >> >> -- >> Thanks, >> >> Steve
| |