Received-SPF: pass (google.com: domain of linux-kernel+bounces-155839-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <ZhgDTj1nwLEtSd9h@neat> <1166494b-3e34-4919-9998-141540a948b3@embeddedor.com>
In-Reply-To: <1166494b-3e34-4919-9998-141540a948b3@embeddedor.com>
From: Steve French <smfrench@gmail.com>
Date: Tue, 23 Apr 2024 15:15:24 -0500
Message-ID: <CAH2r5msZaV1kHqQw8Sb_3wQfGBj4aU+tSCR5E0YJ8fCH6ODB4Q@mail.gmail.com>
Subject: Re: [PATCH][next] smb: smb2pdu.h: Avoid -Wflex-array-member-not-at-end
 warnings
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>, Steve French <sfrench@samba.org>, 
	Paulo Alcantara <pc@manguebit.com>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, 
	Shyam Prasad N <sprasad@microsoft.com>, Tom Talpey <tom@talpey.com>, 
	Bharath SM <bharathsm@microsoft.com>, Namjae Jeon <linkinjeon@kernel.org>, 
	Sergey Senozhatsky <senozhatsky@chromium.org>, linux-cifs@vger.kernel.org, 
	samba-technical@lists.samba.org, linux-kernel@vger.kernel.org, 
	linux-hardening@vger.kernel.org, Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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).

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.

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=E2=80=AFPM 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 containin=
g a flexible array member is not at the end of another structure [-Wflex-ar=
ray-member-not-at-end]
> >
> > Link: https://gist.github.com/GustavoARSilva/772526a39be3dd4db39e71497f=
0a9893 [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 belo=
w. */
> > +     __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