diff mbox series

cifs: fix bi-directional fsctl passthrough calls

Message ID 20190415021352.30854-2-lsahlber@redhat.com
State New
Headers show
Series cifs: fix bi-directional fsctl passthrough calls | expand

Commit Message

Ronnie Sahlberg April 15, 2019, 2:13 a.m. UTC 
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(-)

Comments

Tom Talpey April 16, 2019, 8:30 p.m. UTC | #1 
> -----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
Steve French April 16, 2019, 8:41 p.m. UTC | #2 
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
>
ronnie sahlberg April 17, 2019, 6:36 a.m. UTC | #3 
On Wed, Apr 17, 2019 at 6:31 AM 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]?

Yes. It will always 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
>
diff mbox series

Patch

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) {
+			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;
 		}