MIME-Version: 1.0
References: <20230215000832.never.591-kees@kernel.org>
In-Reply-To: <20230215000832.never.591-kees@kernel.org>
From:   Steve French <smfrench@gmail.com>
Date:   Thu, 16 Feb 2023 23:35:42 -0600
Message-ID: <CAH2r5mv5uKRJtq5buaDzmW5D61gv_E5P=xn5kxjbTwdBa33nsQ@mail.gmail.com>
Subject: Re: [PATCH] cifs: Convert struct fealist away from 1-element array
To:     Kees Cook <keescook@chromium.org>
Cc:     Steve French <sfrench@samba.org>, Paulo Alcantara <pc@cjr.nz>,
        Ronnie Sahlberg <lsahlber@redhat.com>,
        Shyam Prasad N <sprasad@microsoft.com>,
        Tom Talpey <tom@talpey.com>, linux-cifs@vger.kernel.org,
        samba-technical@lists.samba.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

merged into cifs-2.6.git for-next

On Tue, Feb 14, 2023 at 6:16 PM Kees Cook <keescook@chromium.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].
>
> 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@samba.org>
> Cc: Paulo Alcantara <pc@cjr.nz>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>
> Cc: Shyam Prasad N <sprasad@microsoft.com>
> Cc: Tom Talpey <tom@talpey.com>
> Cc: linux-cifs@vger.kernel.org
> Cc: samba-technical@lists.samba.org
> Signed-off-by: Kees Cook <keescook@chromium.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);
> +       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
>


-- 
Thanks,

Steve