| 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: <4defe080-d006-44b8-98ac-b8822a678d70@embeddedor.com>
Date: Wed, 24 Apr 2024 07:08:32 +1000
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Steve French <smfrench@...il.com>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>,
Steve French <sfrench@...ba.org>, Paulo Alcantara <pc@...guebit.com>,
Ronnie Sahlberg <ronniesahlberg@...il.com>,
Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>,
Bharath SM <bharathsm@...rosoft.com>, Namjae Jeon <linkinjeon@...nel.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>, linux-cifs@...r.kernel.org,
samba-technical@...ts.samba.org, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH][next] smb: smb2pdu.h: Avoid
-Wflex-array-member-not-at-end warnings
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@...nel.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@...eddedor.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@...nel.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
Powered by blists - more mailing lists