CIFS: fix FSCTL_SET_REPARSE_POINT SMB2_ioctl() call

Commit Message

Steve French March 5, 2019, 6:14 a.m. UTC

merged Aurelien's small patch (attached) into cifs-2.6.git for-next

Comments

ronnie sahlberg March 5, 2019, 6:56 a.m. UTC | #1

Reviewed-by: Ronnie Sahlberg <[e-mail]>

On Tue, Mar 5, 2019 at 4:16 PM Steve French <[e-mail]> wrote:
>
> merged Aurelien's small patch (attached) into cifs-2.6.git for-next
>
> --
> Thanks,
>
> Steve

Aurélien Aptel March 5, 2019, 11:47 a.m. UTC | #2

Steve French <[e-mail]> writes:
> Without this change the ioctl() fails with INVALID_PARAMETER.
> Since SET_REPARSE_POINT has no output, set the max output response
> size to zero.

Note that this is more of a work around, I didn't intend to send this
yet. I haven't tried to figure out which cases we were violating and
where was the underlying issue.

> If either InputCount, MaxInputResponse, or MaxOutputResponse is
> greater than Connection.MaxTransactSize, the server SHOULD<306> fail
> the request with STATUS_INVALID_PARAMETER.

possible

> The server MUST fail the request with STATUS_INVALID_PARAMETER in the following cases:
>
> * If InputOffset is greater than zero but less than (size of SMB2
>   header + size of the SMB2 IOCTL request not including Buffer) or
>   if InputOffset is greater than (size of SMB2 header + size of the
>   SMB2 IOCTL request).

input related, nope

> * If OutputOffset is greater than zero but less than (size of SMB2
>   header + size of the SMB2 IOCTL request not including Buffer) or if
>   OutputOffset is greater than (size of SMB2 header + size of the SMB2
>   IOCTL request).

doesn't involve MaxOutputResponse, nope

> * If (InputOffset + InputCount) is greater than (size of SMB2 header +
>   size of the SMB2 IOCTL request).

input related, nope

> * If (OutputOffset + OutputCount) is greater than (size of SMB2 header
>   + size of the SMB2 IOCTL request).

doesn't involve MaxOutputResponse, nope

> * If OutputCount is greater than zero and OutputOffset is less
>   than (InputOffset + InputCount).

doesn't involve MaxOutputResponse, nope

So it seems to me only possible explanation is the first, i.e.
MaxOutputResponse > Connection.MaxTransactSize, somehow.

Aurélien Aptel March 5, 2019, 3 p.m. UTC | #3

Aurélien Aptel <[e-mail]> writes:
> So it seems to me only possible explanation is the first, i.e.
> MaxOutputResponse > Connection.MaxTransactSize, somehow.

So I've dumped a few fields:

Connection.MaxTransactSize is set by the server in NEGPROT response.

Against my WS2016, MaxTransactSize = 8388608

But we do this:

	server->maxBuf = min_t(unsigned int, le32_to_cpu(rsp->MaxTransactSize),
			       SMB2_MAX_BUFFER_SIZE);

Since SMB2_MAX_BUFFER_SIZE = 65536, maxBuf is effectively SMB2_MAX_BUFFER_SIZE

And finally, the value used for ioctl MaxOutputResponse is
CIFSMaxBufSize, which is 16384.

I've tried setting MaxOutputResponse to server->maxBuf or
Connection.MaxTransactSize but it always fails the same way... so I
don't know. I guess it must say somewhere that ioctl() with no output
should set MaxOutputResponse to zero?

Aurélien Aptel March 5, 2019, 3:40 p.m. UTC | #4

Aurélien Aptel <[e-mail]> writes:
> I've tried setting MaxOutputResponse to server->maxBuf or
> Connection.MaxTransactSize but it always fails the same way... so I
> don't know. I guess it must say somewhere that ioctl() with no output
> should set MaxOutputResponse to zero?

I've just checked what Windows does and it seems to be that as
well. Creating a symlink (mklink link target) makes an
ioctl(SET_REPARSE_POINT) with MaxOutputResponse set to 0.

So I would keep this patch as is.

Tom Talpey March 5, 2019, 3:49 p.m. UTC | #5

> -----Original Message-----
> From: [e-mail] <[e-mail]> On
> Behalf Of Aurélien Aptel
> Sent: Tuesday, March 5, 2019 10:41 AM
> To: Steve French <[e-mail]>; CIFS <[e-mail]>
> Subject: Re: [PATCH] CIFS: fix FSCTL_SET_REPARSE_POINT SMB2_ioctl() call
> 
> Aurélien Aptel <[e-mail]> writes:
> > I've tried setting MaxOutputResponse to server->maxBuf or
> > Connection.MaxTransactSize but it always fails the same way... so I
> > don't know. I guess it must say somewhere that ioctl() with no output
> > should set MaxOutputResponse to zero?
> 
> I've just checked what Windows does and it seems to be that as
> well. Creating a symlink (mklink link target) makes an
> ioctl(SET_REPARSE_POINT) with MaxOutputResponse set to 0.
> 
> So I would keep this patch as is.

Remember that in addition to the SMB server validation, the request goes
to the filesystem, which has its own validation and error status returns. The
STATUS_INVALID_PARAMETER can come back from any layer.

If the MaxOutputResponse != 0 set_reparse_point behavior isn't covered
in either MS-SMB2 or MS-FSA, I'm happy to take a doc bug.

Tom.

Patch

From 778d81b65e4d596251943002522d94a7c6fbcf69 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <[e-mail]>
Date: Mon, 4 Mar 2019 18:50:18 +0100
Subject: [PATCH] CIFS: fix FSCTL_SET_REPARSE_POINT SMB2_ioctl() call

Without this change the ioctl() fails with INVALID_PARAMETER.
Since SET_REPARSE_POINT has no output, set the max output response
size to zero.

[MS-SMB2] reads 3.3.5.15 Receiving an SMB2 IOCTL Request

If either InputCount, MaxInputResponse, or MaxOutputResponse is
greater than Connection.MaxTransactSize, the server SHOULD<306> fail
the request with STATUS_INVALID_PARAMETER.

The server MUST fail the request with STATUS_INVALID_PARAMETER in the following cases:

* If InputOffset is greater than zero but less than (size of SMB2
  header + size of the SMB2 IOCTL request not including Buffer) or
  if InputOffset is greater than (size of SMB2 header + size of the
  SMB2 IOCTL request).

* If OutputOffset is greater than zero but less than (size of SMB2
  header + size of the SMB2 IOCTL request not including Buffer) or if
  OutputOffset is greater than (size of SMB2 header + size of the SMB2
  IOCTL request).

* If (InputOffset + InputCount) is greater than (size of SMB2 header +
  size of the SMB2 IOCTL request).

* If (OutputOffset + OutputCount) is greater than (size of SMB2 header
  + size of the SMB2 IOCTL request).

* If OutputCount is greater than zero and OutputOffset is less
  than (InputOffset + InputCount).

Signed-off-by: Aurelien Aptel <[e-mail]>
Signed-off-by: Steve French <[e-mail]>
---
 fs/cifs/smb2pdu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 733021566356..cacdf9bf9ef3 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -2539,7 +2539,10 @@  SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid,
 	 * in responses (except for read responses which can be bigger.
 	 * We may want to bump this limit up
 	 */
-	req->MaxOutputResponse = cpu_to_le32(CIFSMaxBufSize);
+	if (opcode == FSCTL_SET_REPARSE_POINT)
+		req->MaxOutputResponse = cpu_to_le32(0);
+	else
+		req->MaxOutputResponse = cpu_to_le32(CIFSMaxBufSize);
 
 	if (is_fsctl)
 		req->Flags = cpu_to_le32(SMB2_0_IOCTL_IS_FSCTL);
-- 
2.17.1