Re: [PATCH] cifs: fix bi-directional fsctl passthrough calls

[Steve French <smfrench@gmail.com>]

le32_to_cpu() returns unsigned - see below from
uapi/linux/byteorder/little_endian.h:

     #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))

On Tue, Apr 16, 2019 at 3:30 PM Tom Talpey <ttalpey@microsoft.com> wrote:
>
> > -----Original Message-----
> > From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> > Behalf Of Ronnie Sahlberg
> > Sent: Sunday, April 14, 2019 10:14 PM
> > To: linux-cifs <linux-cifs@vger.kernel.org>
> > Cc: Steve French <smfrench@gmail.com>; Ronnie Sahlberg
> > <lsahlber@redhat.com>
> > Subject: [PATCH] cifs: fix bi-directional fsctl passthrough calls
> >
> > SMB2 Ioctl responses from servers may respond with both the request blob
> > from
> > the client followed by the actual reply blob for ioctls that are bi-directional.
> >
> > In that case we can not assume that the reply blob comes immediately after
> > the
> > ioctl response structure.
> >
> > This fixes FSCTLs such as SMB2:FSCTL_QUERY_ALLOCATED_RANGES
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/smb2ops.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 0b7e2be2a781..2824b97c5869 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -1462,12 +1462,19 @@ smb2_ioctl_query_info(const unsigned int xid,
> >               io_rsp = (struct smb2_ioctl_rsp *)rsp_iov[1].iov_base;
> >               if (le32_to_cpu(io_rsp->OutputCount) <
> > qi.input_buffer_length)
> >                       qi.input_buffer_length = le32_to_cpu(io_rsp-
> > >OutputCount);
> > +             if (qi.input_buffer_length > 0 &&
> > +                 le32_to_cpu(io_rsp->OutputOffset) + qi.input_buffer_length
> > > rsp_iov[1].iov_len) {
>
> I hope this isn't a dumb question, but I can't readily figure it out due to
> multiple layers of #include nesting. Is le32_to_cpu() signed or unsigned?
> If it's signed, this validation could still be attacked.
>
> And second question, is the fsctl output payload always guaranteed to land
> in rsp_iov[1]?
>
> Tom.
>
> > +                     rc = -EFAULT;
> > +                     goto iqinf_exit;
> > +             }
> >               if (copy_to_user(&pqi->input_buffer_length,
> > &qi.input_buffer_length,
> >                                sizeof(qi.input_buffer_length))) {
> >                       rc = -EFAULT;
> >                       goto iqinf_exit;
> >               }
> > -             if (copy_to_user(pqi + 1, &io_rsp[1], qi.input_buffer_length)) {
> > +             if (copy_to_user((char *)pqi + sizeof(struct smb_query_info),
> > +                              (char *)io_rsp + le32_to_cpu(io_rsp-
> > >OutputOffset),
> > +                              qi.input_buffer_length)) {
> >                       rc = -EFAULT;
> >                       goto iqinf_exit;
> >               }
> > --
> > 2.13.6
>


-- 
Thanks,

Steve