Re: [PATCH] ksmbd: Replace one-element arrays with flexible-array members

[Tom Talpey]

On 8/20/2024 12:33 PM, Thorsten Blum wrote:
> On 20. Aug 2024, at 16:52, Tom Talpey <tom@xxxxxxxxxx> wrote:
> > On 8/20/2024 10:11 AM, Namjae Jeon wrote:
> > > On Mon, Aug 19, 2024 at 1:22 AM Thorsten Blum <thorsten.blum@xxxxxxxxxx> wrote:
> > > > Replace the deprecated one-element arrays with flexible-array members
> > > > in the structs copychunk_ioctl_req and smb2_ea_info_req.
> > > >
> > > > There are no binary differences after this conversion.
> > > >
> > > > Link: https://github.com/KSPP/linux/issues/79
> > > > Signed-off-by: Thorsten Blum <thorsten.blum@xxxxxxxxxx>
> > > > ---
> > > >   fs/smb/server/smb2pdu.c | 4 ++--
> > > >   fs/smb/server/smb2pdu.h | 4 ++--
> > > >   2 files changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> > > > index 2df1354288e6..83667cb78fa6 100644
> > > > --- a/fs/smb/server/smb2pdu.c
> > > > +++ b/fs/smb/server/smb2pdu.c
> > > > @@ -4580,7 +4580,7 @@ static int smb2_get_ea(struct ksmbd_work *work, struct ksmbd_file *fp,
> > > >          /* single EA entry is requested with given user.* name */
> > > >          if (req->InputBufferLength) {
> > > >                  if (le32_to_cpu(req->InputBufferLength) <
> > > > -                   sizeof(struct smb2_ea_info_req))
> > > > +                   sizeof(struct smb2_ea_info_req) + 1)
> > > We can use <= instead of +1.
> >
> > This is better, but maybe this test was actually not right in
> > the first place.
> >
> > I think a strict "<" is correct here, because the ea name
> > field is a counted array of length EaNameLength. So, it's
> > a layering issue to fail with EINVAL this early in the
> > processing. All that should be checked up front is
> > that a complete smb2_ea_info_req header is present.
>
> Just to clarify before I submit a v2: Is a strict "<" and without "+1"
> correct?

Maybe safest with "<=", because it changes no behavior.

I do think there is an issue with the test, because it is
just checking for one byte of the EaName and not even checking
EaNameLength, and similar for copychunk.

But these are pre-existing protocol parsing matters, not flex
array ones. We can discuss those later.

Tom.

> > > >                          return -EINVAL;
> > > >
> > > >                  ea_req = (struct smb2_ea_info_req *)((char *)req +
> > > > @@ -8083,7 +8083,7 @@ int smb2_ioctl(struct ksmbd_work *work)
> > > >                          goto out;
> > > >                  }
> > > >
> > > > -               if (in_buf_len < sizeof(struct copychunk_ioctl_req)) {
> > > > +               if (in_buf_len < sizeof(struct copychunk_ioctl_req) + 1) {
> > > Ditto.
> >
> > And ditto.
>
> Same here, strict "<" and without "+ 1"? Or just a refactor to "<="
> without changing the condition?
>
> Thanks,
> Thorsten