cifs: fix bi-directional fsctl passthrough calls
diff mbox series

Message ID 20190412030534.23402-1-lsahlber@redhat.com
State New
Headers show
Series
  • cifs: fix bi-directional fsctl passthrough calls
Related show

Commit Message

Ronnie Sahlberg April 12, 2019, 3:05 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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Tom Talpey April 12, 2019, 12:51 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: Thursday, April 11, 2019 11:06 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 | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 0b7e2be2a781..768f35ea63cf 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1467,7 +1467,9 @@ smb2_ioctl_query_info(const unsigned int xid,
>  			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)) {

Shouldn't this validate that OutputOffset, for a length of input_buffer_length, falls completely within the bounds of the response, before copying?


>  			rc = -EFAULT;
>  			goto iqinf_exit;
>  		}
> --
> 2.13.6
ronnie sahlberg April 12, 2019, 1:37 p.m. UTC | #2
On Fri, Apr 12, 2019 at 10:51 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: Thursday, April 11, 2019 11:06 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 | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 0b7e2be2a781..768f35ea63cf 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -1467,7 +1467,9 @@ smb2_ioctl_query_info(const unsigned int xid,
> >                       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)) {
>
> Shouldn't this validate that OutputOffset, for a length of input_buffer_length, falls completely within the bounds of the response, before copying?

I think copy_to_user handles that

>
>
> >                       rc = -EFAULT;
> >                       goto iqinf_exit;
> >               }
> > --
> > 2.13.6
>
Tom Talpey April 12, 2019, 1:53 p.m. UTC | #3
> -----Original Message-----
> From: ronnie sahlberg <ronniesahlberg@gmail.com>
> Sent: Friday, April 12, 2019 9:38 AM
> To: Tom Talpey <ttalpey@microsoft.com>
> Cc: Ronnie Sahlberg <lsahlber@redhat.com>; linux-cifs <linux-
> cifs@vger.kernel.org>; Steve French <smfrench@gmail.com>
> Subject: Re: [PATCH] cifs: fix bi-directional fsctl passthrough calls
> 
> On Fri, Apr 12, 2019 at 10:51 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: Thursday, April 11, 2019 11:06 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 | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > index 0b7e2be2a781..768f35ea63cf 100644
> > > --- a/fs/cifs/smb2ops.c
> > > +++ b/fs/cifs/smb2ops.c
> > > @@ -1467,7 +1467,9 @@ smb2_ioctl_query_info(const unsigned int xid,
> > >                       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)) {
> >
> > Shouldn't this validate that OutputOffset, for a length of input_buffer_length,
> falls completely within the bounds of the response, before copying?
> 
> I think copy_to_user handles that

Absolutely not - copy_to_user cannot validate SMB2 messages, and only handles
user faults (destination buffer not source).

If the server is buggy or evil, OutputOffset might lie outside the response.

If the response payload is shorter than qi.input_buffer_length, this code will
copy kernel memory, or may walk off the end of a kernel page.

Both very bad things?


> 
> >
> >
> > >                       rc = -EFAULT;
> > >                       goto iqinf_exit;
> > >               }
> > > --
> > > 2.13.6
> >

Patch
diff mbox series

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 0b7e2be2a781..768f35ea63cf 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1467,7 +1467,9 @@  smb2_ioctl_query_info(const unsigned int xid,
 			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;
 		}