diff mbox series

smb: client: Fix -Wstringop-overflow issues

Message ID ZK3h3+dHBGONHt+S@work (mailing list archive)
State Mainlined
Commit f1f047bd7ce0d73788e04ac02268060a565f7ecb
Headers show
Series smb: client: Fix -Wstringop-overflow issues | expand

Commit Message

Gustavo A. R. Silva July 11, 2023, 11:12 p.m. UTC 
pSMB->hdr.Protocol is an array of size 4 bytes, hence when the compiler
analyzes this line of code

	parm_data = ((char *) &pSMB->hdr.Protocol) + offset;

it legitimately complains about the fact that offset points outside the
bounds of the array. Notice that the compiler gives priority to the object
as an array, rather than merely the address of one more byte in a structure
to wich offset should be added (which seems to be the actual intention of
the original implementation).

Fix this by explicitly instructing the compiler to treat the code as a
sequence of bytes in struct smb_com_transaction2_spi_req, and not as an
array accessed through pointer notation.

Notice that ((char *)pSMB) + sizeof(pSMB->hdr.smb_buf_length) points to
the same address as ((char *) &pSMB->hdr.Protocol), therefore this results
in no differences in binary output.

Fixes the following -Wstringop-overflow warnings when built s390
architecture with defconfig (GCC 13):
  CC [M]  fs/smb/client/cifssmb.o
In function 'cifs_init_ace',
    inlined from 'posix_acl_to_cifs' at fs/smb/client/cifssmb.c:3046:3,
    inlined from 'cifs_do_set_acl' at fs/smb/client/cifssmb.c:3191:15:
fs/smb/client/cifssmb.c:2987:31: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
 2987 |         cifs_ace->cifs_e_perm = local_ace->e_perm;
      |         ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
In file included from fs/smb/client/cifssmb.c:27:
fs/smb/client/cifspdu.h: In function 'cifs_do_set_acl':
fs/smb/client/cifspdu.h:384:14: note: at offset [7, 11] into destination object 'Protocol' of size 4
  384 |         __u8 Protocol[4];
      |              ^~~~~~~~
In function 'cifs_init_ace',
    inlined from 'posix_acl_to_cifs' at fs/smb/client/cifssmb.c:3046:3,
    inlined from 'cifs_do_set_acl' at fs/smb/client/cifssmb.c:3191:15:
fs/smb/client/cifssmb.c:2988:30: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
 2988 |         cifs_ace->cifs_e_tag =  local_ace->e_tag;
      |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
fs/smb/client/cifspdu.h: In function 'cifs_do_set_acl':
fs/smb/client/cifspdu.h:384:14: note: at offset [6, 10] into destination object 'Protocol' of size 4
  384 |         __u8 Protocol[4];
      |              ^~~~~~~~

This helps with the ongoing efforts to globally enable
-Wstringop-overflow.

Link: https://github.com/KSPP/linux/issues/310
Fixes: dc1af4c4b472 ("cifs: implement set acl method")
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 fs/smb/client/cifssmb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steve French July 12, 2023, 4:30 a.m. UTC | #1 
tentatively merged into cifs-2.6.git for-next pending testing

On Tue, Jul 11, 2023 at 6:20 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> pSMB->hdr.Protocol is an array of size 4 bytes, hence when the compiler
> analyzes this line of code
>
>         parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
>
> it legitimately complains about the fact that offset points outside the
> bounds of the array. Notice that the compiler gives priority to the object
> as an array, rather than merely the address of one more byte in a structure
> to wich offset should be added (which seems to be the actual intention of
> the original implementation).
>
> Fix this by explicitly instructing the compiler to treat the code as a
> sequence of bytes in struct smb_com_transaction2_spi_req, and not as an
> array accessed through pointer notation.
>
> Notice that ((char *)pSMB) + sizeof(pSMB->hdr.smb_buf_length) points to
> the same address as ((char *) &pSMB->hdr.Protocol), therefore this results
> in no differences in binary output.
>
> Fixes the following -Wstringop-overflow warnings when built s390
> architecture with defconfig (GCC 13):
>   CC [M]  fs/smb/client/cifssmb.o
> In function 'cifs_init_ace',
>     inlined from 'posix_acl_to_cifs' at fs/smb/client/cifssmb.c:3046:3,
>     inlined from 'cifs_do_set_acl' at fs/smb/client/cifssmb.c:3191:15:
> fs/smb/client/cifssmb.c:2987:31: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>  2987 |         cifs_ace->cifs_e_perm = local_ace->e_perm;
>       |         ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> In file included from fs/smb/client/cifssmb.c:27:
> fs/smb/client/cifspdu.h: In function 'cifs_do_set_acl':
> fs/smb/client/cifspdu.h:384:14: note: at offset [7, 11] into destination object 'Protocol' of size 4
>   384 |         __u8 Protocol[4];
>       |              ^~~~~~~~
> In function 'cifs_init_ace',
>     inlined from 'posix_acl_to_cifs' at fs/smb/client/cifssmb.c:3046:3,
>     inlined from 'cifs_do_set_acl' at fs/smb/client/cifssmb.c:3191:15:
> fs/smb/client/cifssmb.c:2988:30: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>  2988 |         cifs_ace->cifs_e_tag =  local_ace->e_tag;
>       |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> fs/smb/client/cifspdu.h: In function 'cifs_do_set_acl':
> fs/smb/client/cifspdu.h:384:14: note: at offset [6, 10] into destination object 'Protocol' of size 4
>   384 |         __u8 Protocol[4];
>       |              ^~~~~~~~
>
> This helps with the ongoing efforts to globally enable
> -Wstringop-overflow.
>
> Link: https://github.com/KSPP/linux/issues/310
> Fixes: dc1af4c4b472 ("cifs: implement set acl method")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  fs/smb/client/cifssmb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index 19f7385abeec..9dee267f1893 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -3184,7 +3184,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>         param_offset = offsetof(struct smb_com_transaction2_spi_req,
>                                 InformationLevel) - 4;
>         offset = param_offset + params;
> -       parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
> +       parm_data = ((char *)pSMB) + sizeof(pSMB->hdr.smb_buf_length) + offset;
>         pSMB->ParameterOffset = cpu_to_le16(param_offset);
>
>         /* convert to on the wire format for POSIX ACL */
> --
> 2.34.1
>
Kees Cook July 13, 2023, 12:01 a.m. UTC | #2 
On Tue, Jul 11, 2023 at 05:12:31PM -0600, Gustavo A. R. Silva wrote:
> pSMB->hdr.Protocol is an array of size 4 bytes, hence when the compiler
> analyzes this line of code
> 
> 	parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
> 
> it legitimately complains about the fact that offset points outside the
> bounds of the array. Notice that the compiler gives priority to the object
> as an array, rather than merely the address of one more byte in a structure
> to wich offset should be added (which seems to be the actual intention of
> the original implementation).
> 
> Fix this by explicitly instructing the compiler to treat the code as a
> sequence of bytes in struct smb_com_transaction2_spi_req, and not as an
> array accessed through pointer notation.
> 
> Notice that ((char *)pSMB) + sizeof(pSMB->hdr.smb_buf_length) points to
> the same address as ((char *) &pSMB->hdr.Protocol), therefore this results
> in no differences in binary output.
> 
> Fixes the following -Wstringop-overflow warnings when built s390
> architecture with defconfig (GCC 13):
>   CC [M]  fs/smb/client/cifssmb.o
> In function 'cifs_init_ace',
>     inlined from 'posix_acl_to_cifs' at fs/smb/client/cifssmb.c:3046:3,
>     inlined from 'cifs_do_set_acl' at fs/smb/client/cifssmb.c:3191:15:
> fs/smb/client/cifssmb.c:2987:31: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>  2987 |         cifs_ace->cifs_e_perm = local_ace->e_perm;
>       |         ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> In file included from fs/smb/client/cifssmb.c:27:
> fs/smb/client/cifspdu.h: In function 'cifs_do_set_acl':
> fs/smb/client/cifspdu.h:384:14: note: at offset [7, 11] into destination object 'Protocol' of size 4
>   384 |         __u8 Protocol[4];
>       |              ^~~~~~~~
> In function 'cifs_init_ace',
>     inlined from 'posix_acl_to_cifs' at fs/smb/client/cifssmb.c:3046:3,
>     inlined from 'cifs_do_set_acl' at fs/smb/client/cifssmb.c:3191:15:
> fs/smb/client/cifssmb.c:2988:30: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>  2988 |         cifs_ace->cifs_e_tag =  local_ace->e_tag;
>       |         ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> fs/smb/client/cifspdu.h: In function 'cifs_do_set_acl':
> fs/smb/client/cifspdu.h:384:14: note: at offset [6, 10] into destination object 'Protocol' of size 4
>   384 |         __u8 Protocol[4];
>       |              ^~~~~~~~
> 
> This helps with the ongoing efforts to globally enable
> -Wstringop-overflow.
> 
> Link: https://github.com/KSPP/linux/issues/310
> Fixes: dc1af4c4b472 ("cifs: implement set acl method")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  fs/smb/client/cifssmb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
> index 19f7385abeec..9dee267f1893 100644
> --- a/fs/smb/client/cifssmb.c
> +++ b/fs/smb/client/cifssmb.c
> @@ -3184,7 +3184,7 @@ int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
>  	param_offset = offsetof(struct smb_com_transaction2_spi_req,
>  				InformationLevel) - 4;
>  	offset = param_offset + params;
> -	parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
> +	parm_data = ((char *)pSMB) + sizeof(pSMB->hdr.smb_buf_length) + offset;
>  	pSMB->ParameterOffset = cpu_to_le16(param_offset);
>  
>  	/* convert to on the wire format for POSIX ACL */

This looks correct, though looking at this code I think some serious
comments are needed to describe _why_ these offsets are calculated the
way the are. The only dynamic part of parm_data is name_len, and could
just as easily be calculated as:

	parm_data = pSMB->FileName + name_len;

which is MUCH more readable. But, yes, the above patch does result in
the same binary code.

Reviewed-by: Kees Cook <keescook@chromium.org>
diff mbox series

Patch

diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 19f7385abeec..9dee267f1893 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -3184,7 +3184,7 @@  int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
 	param_offset = offsetof(struct smb_com_transaction2_spi_req,
 				InformationLevel) - 4;
 	offset = param_offset + params;
-	parm_data = ((char *) &pSMB->hdr.Protocol) + offset;
+	parm_data = ((char *)pSMB) + sizeof(pSMB->hdr.smb_buf_length) + offset;
 	pSMB->ParameterOffset = cpu_to_le16(param_offset);
 
 	/* convert to on the wire format for POSIX ACL */