[PATCH 5.10.y] cifs: fix off-by-one in SMB2_query_info_init()

[PATCH 5.10.y] cifs: fix off-by-one in SMB2_query_info_init()

[Harshit Mogalapalli]

Bug: After mounting the cifs fs, it complains with Resource temporarily
unavailable messages.

[root@vm1 xfstests-dev]# ./check -g quick -s smb3
TEST_DEV=//<SERVER_IP>/TEST is mounted but not a type cifs filesystem
[root@vm1 xfstests-dev]# df
df: /mnt/test: Resource temporarily unavailable

Paul's analysis of the bug:

	Bug is related to an off-by-one in smb2_set_next_command() when
	the client attempts to pad SMB2_QUERY_INFO request -- since it isn't
	8 byte aligned -- even though smb2_query_info_compound() doesn't
	provide an extra iov for such padding.

	v5.10.y doesn't have

        eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays")

	and the commit does

		if (unlikely(check_add_overflow(input_len, sizeof(*req), &len) ||
			     len > CIFSMaxBufSize))
			return -EINVAL;

	so sizeof(*req) will wrongly include the extra byte from
	smb2_query_info_req::Buffer making @len unaligned and therefore causing
	OOB in smb2_set_next_command().

Fixes: 203a412e52b5 ("smb: client: fix OOB in SMB2_query_info_init()")
Suggested-by: Paulo Alcantara <pc@manguebit.com>
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
This patch is only for v5.10.y stable kernel.
I have tested the patched kernel, after mounting it doesn't become
unavailable.

Context:
[1] https://lore.kernel.org/all/CAH2r5mv2ipr4KJfMDXwHgq9L+kGdnRd1C2svcM=PCoDjA7uALA@mail.gmail.com/#t

Note to Greg: This is alternative way to fix by not taking commit
eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with
flex-arrays").
before applying this patch a patch in the queue needs to be removed: https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-5.10/smb3-replace-smb2pdu-1-element-arrays-with-flex-arrays.patch

As I have stated in [1] I am unsure the which is the best way, but this
commit eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with
flex-arrays") is not in 5.15.y so I think we shouldn't queue it in
5.10.y
---
 fs/cifs/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 76679dc4e6328..514e2cf44d951 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -3379,7 +3379,7 @@ SMB2_query_info_init(struct cifs_tcon *tcon, struct TCP_Server_Info *server,
 
 	iov[0].iov_base = (char *)req;
 	/* 1 for Buffer */
-	iov[0].iov_len = len;
+	iov[0].iov_len = len - 1;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 5.10.y] cifs: fix off-by-one in SMB2_query_info_init()

[kovalev]

29.01.2024 08:43, Harshit Mogalapalli wrote:
> This patch is only for v5.10.y stable kernel.
> I have tested the patched kernel, after mounting it doesn't become
> unavailable.
>
> Context:
> [1] https://lore.kernel.org/all/CAH2r5mv2ipr4KJfMDXwHgq9L+kGdnRd1C2svcM=PCoDjA7uALA@mail.gmail.com/#t
>
> Note to Greg: This is alternative way to fix by not taking commit
> eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with
> flex-arrays").
> before applying this patch a patch in the queue needs to be removed: https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-5.10/smb3-replace-smb2pdu-1-element-arrays-with-flex-arrays.patch
Maybe I don't understand something, but isn't there a goal when fixing 
bugs to keep the code of stable branches with upstream code as much as 
possible? Otherwise, the following fixes will not be compatible..

-- 
Regards,
Vasiliy Kovalev

Re: [PATCH 5.10.y] cifs: fix off-by-one in SMB2_query_info_init()

[Harshit Mogalapalli]

Hi Kovalev,

On 29/01/24 1:49 pm, kovalev@altlinux.org wrote:
> 29.01.2024 08:43, Harshit Mogalapalli wrote:
>> This patch is only for v5.10.y stable kernel.
>> I have tested the patched kernel, after mounting it doesn't become
>> unavailable.
>>
>> Context:
>> [1] 
>> https://lore.kernel.org/all/CAH2r5mv2ipr4KJfMDXwHgq9L+kGdnRd1C2svcM=PCoDjA7uALA@mail.gmail.com/#t
>>
>> Note to Greg: This is alternative way to fix by not taking commit
>> eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with
>> flex-arrays").
>> before applying this patch a patch in the queue needs to be removed: 
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-5.10/smb3-replace-smb2pdu-1-element-arrays-with-flex-arrays.patch
> Maybe I don't understand something, but isn't there a goal when fixing 
> bugs to keep the code of stable branches with upstream code as much as 
> possible? Otherwise, the following fixes will not be compatible..

I agree, but at the same time we also should observe this:
eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays") 
is not in 5.15.y so we probably shouldn't queue it up for 5.10.y.

Ref from stable kernel rules document:
https://www.kernel.org/doc/html/v6.7/process/stable-kernel-rules.html#:~:text=When%20using%20option,to%205.15.y.
(Just above Option 1 description)

Thanks,
Harshit
> 

Re: [PATCH 5.10.y] cifs: fix off-by-one in SMB2_query_info_init()

[Greg KH]

On Mon, Jan 29, 2024 at 09:57:40PM +0530, Harshit Mogalapalli wrote:
> Hi Kovalev,
> 
> On 29/01/24 1:49 pm, kovalev@altlinux.org wrote:
> > 29.01.2024 08:43, Harshit Mogalapalli wrote:
> > > This patch is only for v5.10.y stable kernel.
> > > I have tested the patched kernel, after mounting it doesn't become
> > > unavailable.
> > > 
> > > Context:
> > > [1] https://lore.kernel.org/all/CAH2r5mv2ipr4KJfMDXwHgq9L+kGdnRd1C2svcM=PCoDjA7uALA@mail.gmail.com/#t
> > > 
> > > Note to Greg: This is alternative way to fix by not taking commit
> > > eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with
> > > flex-arrays").
> > > before applying this patch a patch in the queue needs to be removed: https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-5.10/smb3-replace-smb2pdu-1-element-arrays-with-flex-arrays.patch
> > Maybe I don't understand something, but isn't there a goal when fixing
> > bugs to keep the code of stable branches with upstream code as much as
> > possible? Otherwise, the following fixes will not be compatible..
> 
> I agree, but at the same time we also should observe this:
> eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays") is
> not in 5.15.y so we probably shouldn't queue it up for 5.10.y.

It is queued up for 5.10, but not 5.15 for some reason?

confused,

greg k-h

Re: [PATCH 5.10.y] cifs: fix off-by-one in SMB2_query_info_init()

[Harshit Mogalapalli]

On 29/01/24 10:07 pm, Greg KH wrote:
> On Mon, Jan 29, 2024 at 09:57:40PM +0530, Harshit Mogalapalli wrote:
>> Hi Kovalev,
>>
>> On 29/01/24 1:49 pm, kovalev@altlinux.org wrote:
>>> 29.01.2024 08:43, Harshit Mogalapalli wrote:
>>>> This patch is only for v5.10.y stable kernel.
>>>> I have tested the patched kernel, after mounting it doesn't become
>>>> unavailable.
>>>>
>>>> Context:
>>>> [1] https://lore.kernel.org/all/CAH2r5mv2ipr4KJfMDXwHgq9L+kGdnRd1C2svcM=PCoDjA7uALA@mail.gmail.com/#t
>>>>
>>>> Note to Greg: This is alternative way to fix by not taking commit
>>>> eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with
>>>> flex-arrays").
>>>> before applying this patch a patch in the queue needs to be removed: https://git.kernel.org/pub/scm/linux/kernel/git/stable/stable-queue.git/tree/queue-5.10/smb3-replace-smb2pdu-1-element-arrays-with-flex-arrays.patch
>>> Maybe I don't understand something, but isn't there a goal when fixing
>>> bugs to keep the code of stable branches with upstream code as much as
>>> possible? Otherwise, the following fixes will not be compatible..
>>
>> I agree, but at the same time we also should observe this:
>> eb3e28c1e89b ("smb3: Replace smb2pdu 1-element arrays with flex-arrays") is
>> not in 5.15.y so we probably shouldn't queue it up for 5.10.y.
> 
Hi Greg,

> It is queued up for 5.10, but not 5.15 for some reason?
> 

Context: 
https://lore.kernel.org/all/472d92aa-1b49-43c9-a91f-80dfc8f25ad3@oracle.com/

I think the above commit eb3e28c1e89b ("smb3: Replace smb2pdu 1-element 
arrays with flex-arrays") got queued up from the above backport.

I did try applying on 5.15.y but I noticed many conflicts, so I asked 
which is the preferred way:
  1. Resolving conflicts by backporting above commit(flex-arrays one), 
which touches more code.
  2. Or go with one line change.

I thought one liner is is a simpler change although it is not a upstream 
commit. Steve French also agreed with approach 2(which Paul suggested 
here [1], so I made patch(approach 2) for 5.15.y and 5.10.y and tested 
both after patching, they work.

[1] 
https://lore.kernel.org/all/446860c571d0699ed664175262a9e84b@manguebit.com/

Thanks,
Harshit

> confused,
> 
> greg k-h