[PATCH][CIFS] Clarify SMB1 code for rename open file

[PATCH][CIFS] Clarify SMB1 code for rename open file

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

And one more trivial coverity issue related patch ...
(with fewer old issues like this, in the future it will be easier
to spot important new ones that tools like this report)

Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for SMB1 RenameOpenFile. This changeset
doesn't change the address but makes it slightly clearer.

Addresses-Coverity: 711521 ("Out of bounds write")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifssmb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 0863238ddd20..16bd4cf3bceb 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2767,7 +2767,8 @@ int CIFSSMBRenameOpenFile(const unsigned int
xid, struct cifs_tcon *pTcon,
  param_offset = offsetof(struct smb_com_transaction2_sfi_req, Fid) - 4;
  offset = param_offset + params;

- data_offset = (char *) (&pSMB->hdr.Protocol) + offset;
+ /* SMB offsets are from the beginning of SMB which is 4 bytes in,
after RFC1001 field */
+ data_offset = (char *)(pSMB) + offset + 4;
  rename_info = (struct set_file_rename *) data_offset;
  pSMB->MaxParameterCount = cpu_to_le16(2);
  pSMB->MaxDataCount = cpu_to_le16(1000); /* BB find max SMB from sess */

-- 
Thanks,

Steve

[-- Attachment #2: 0001-CIFS-Clarify-SMB1-code-for-rename-open-file.patch --]
[-- Type: text/x-patch, Size: 1416 bytes --]

From a2c6b5f95226d58f85864f5a30e892b5ec885bd6 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Wed, 7 Jul 2021 13:34:47 -0500
Subject: [PATCH] CIFS: Clarify SMB1 code for rename open file

Coverity also complains about the way we calculate the offset
(starting from the address of a 4 byte array within the
header structure rather than from the beginning of the struct
plus 4 bytes) for SMB1 RenameOpenFile. This changeset
doesn't change the address but makes it slightly clearer.

Addresses-Coverity: 711521 ("Out of bounds write")
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifssmb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 0863238ddd20..16bd4cf3bceb 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -2767,7 +2767,8 @@ int CIFSSMBRenameOpenFile(const unsigned int xid, struct cifs_tcon *pTcon,
 	param_offset = offsetof(struct smb_com_transaction2_sfi_req, Fid) - 4;
 	offset = param_offset + params;
 
-	data_offset = (char *) (&pSMB->hdr.Protocol) + offset;
+	/* SMB offsets are from the beginning of SMB which is 4 bytes in, after RFC1001 field */
+	data_offset = (char *)(pSMB) + offset + 4;
 	rename_info = (struct set_file_rename *) data_offset;
 	pSMB->MaxParameterCount = cpu_to_le16(2);
 	pSMB->MaxDataCount = cpu_to_le16(1000); /* BB find max SMB from sess */
-- 
2.30.2

Re: [PATCH][CIFS] Clarify SMB1 code for rename open file

[Paulo Alcantara]

Steve French <smfrench@gmail.com> writes:

> And one more trivial coverity issue related patch ...
> (with fewer old issues like this, in the future it will be easier
> to spot important new ones that tools like this report)
>
> Coverity also complains about the way we calculate the offset
> (starting from the address of a 4 byte array within the
> header structure rather than from the beginning of the struct
> plus 4 bytes) for SMB1 RenameOpenFile. This changeset
> doesn't change the address but makes it slightly clearer.
>
> Addresses-Coverity: 711521 ("Out of bounds write")
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/cifssmb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>