* [PATCH] cifs: fix bi-directional fsctl passthrough calls
@ 2019-04-16 23:01 Steve French
0 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2019-04-16 23:01 UTC (permalink / raw)
To: ronnie sahlberg, CIFS
[-- Attachment #1: Type: text/plain, Size: 64 bytes --]
Trivial fix up for compile/sparse warning
--
Thanks,
Steve
[-- Attachment #2: 0001-cifs-fix-bi-directional-fsctl-passthrough-calls.patch --]
[-- Type: text/x-patch, Size: 1760 bytes --]
From 2dce1073a0008e46673684cc59b990504fc66235 Mon Sep 17 00:00:00 2001
From: Ronnie Sahlberg <lsahlber@redhat.com>
Date: Mon, 15 Apr 2019 12:13:52 +1000
Subject: [PATCH 1/2] 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>
Signed-off-by: Steve French <stfrench@microsoft.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 08ff044fbb4b..841ce7e1fb72 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((void __user *)pqi + sizeof(struct smb_query_info),
+ (const void *)io_rsp + le32_to_cpu(io_rsp->OutputOffset),
+ qi.input_buffer_length)) {
rc = -EFAULT;
goto iqinf_exit;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* fix bidirectional fsctl passthrough
@ 2019-04-15 2:13 Ronnie Sahlberg
2019-04-15 2:13 ` [PATCH] cifs: fix bi-directional fsctl passthrough calls Ronnie Sahlberg
0 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-04-15 2:13 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French
Steve, Tom
Please find an updated version that adds bounds checking as pointer out by Tom.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-15 2:13 fix bidirectional fsctl passthrough Ronnie Sahlberg
@ 2019-04-15 2:13 ` Ronnie Sahlberg
2019-04-16 20:30 ` Tom Talpey
0 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-04-15 2:13 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Ronnie Sahlberg
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) {
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-15 2:13 ` [PATCH] cifs: fix bi-directional fsctl passthrough calls Ronnie Sahlberg
@ 2019-04-16 20:30 ` Tom Talpey
2019-04-16 20:41 ` Steve French
2019-04-17 6:36 ` ronnie sahlberg
0 siblings, 2 replies; 9+ messages in thread
From: Tom Talpey @ 2019-04-16 20:30 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French
> -----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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-16 20:30 ` Tom Talpey
@ 2019-04-16 20:41 ` Steve French
2019-04-17 6:36 ` ronnie sahlberg
1 sibling, 0 replies; 9+ messages in thread
From: Steve French @ 2019-04-16 20:41 UTC (permalink / raw)
To: Tom Talpey; +Cc: Ronnie Sahlberg, linux-cifs
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-16 20:30 ` Tom Talpey
2019-04-16 20:41 ` Steve French
@ 2019-04-17 6:36 ` ronnie sahlberg
1 sibling, 0 replies; 9+ messages in thread
From: ronnie sahlberg @ 2019-04-17 6:36 UTC (permalink / raw)
To: Tom Talpey; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] cifs: fix bi-directional fsctl passthrough calls
@ 2019-04-12 3:05 Ronnie Sahlberg
2019-04-12 12:51 ` Tom Talpey
0 siblings, 1 reply; 9+ messages in thread
From: Ronnie Sahlberg @ 2019-04-12 3:05 UTC (permalink / raw)
To: linux-cifs; +Cc: Steve French, Ronnie Sahlberg
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)) {
rc = -EFAULT;
goto iqinf_exit;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-12 3:05 Ronnie Sahlberg
@ 2019-04-12 12:51 ` Tom Talpey
2019-04-12 13:37 ` ronnie sahlberg
0 siblings, 1 reply; 9+ messages in thread
From: Tom Talpey @ 2019-04-12 12:51 UTC (permalink / raw)
To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French
> -----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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-12 12:51 ` Tom Talpey
@ 2019-04-12 13:37 ` ronnie sahlberg
2019-04-12 13:53 ` Tom Talpey
0 siblings, 1 reply; 9+ messages in thread
From: ronnie sahlberg @ 2019-04-12 13:37 UTC (permalink / raw)
To: Tom Talpey; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] cifs: fix bi-directional fsctl passthrough calls
2019-04-12 13:37 ` ronnie sahlberg
@ 2019-04-12 13:53 ` Tom Talpey
0 siblings, 0 replies; 9+ messages in thread
From: Tom Talpey @ 2019-04-12 13:53 UTC (permalink / raw)
To: ronnie sahlberg; +Cc: Ronnie Sahlberg, linux-cifs, Steve French
> -----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
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-04-17 6:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16 23:01 [PATCH] cifs: fix bi-directional fsctl passthrough calls Steve French
-- strict thread matches above, loose matches on Subject: below --
2019-04-15 2:13 fix bidirectional fsctl passthrough Ronnie Sahlberg
2019-04-15 2:13 ` [PATCH] cifs: fix bi-directional fsctl passthrough calls Ronnie Sahlberg
2019-04-16 20:30 ` Tom Talpey
2019-04-16 20:41 ` Steve French
2019-04-17 6:36 ` ronnie sahlberg
2019-04-12 3:05 Ronnie Sahlberg
2019-04-12 12:51 ` Tom Talpey
2019-04-12 13:37 ` ronnie sahlberg
2019-04-12 13:53 ` Tom Talpey
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).