| Message ID | 20190412030534.23402-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix bi-directional fsctl passthrough calls | expand |
> -----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
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 >
> -----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 > >
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; }
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(-)