| Message ID | 20190415021352.30854-2-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: 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
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
>
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 --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; }
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(-)