| Message ID | 20240129054342.2472454-1-harshit.m.mogalapalli@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [5.10.y] cifs: fix off-by-one in SMB2_query_info_init() | expand |
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..
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 >
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
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
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; }
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(-)