diff mbox series

smb: client: Fix -Wstringop-overflow issues

Message ID	ZK3h3+dHBGONHt+S@work
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Date: Tue, 11 Jul 2023 17:12:31 -0600 From: "Gustavo A. R. Silva" <gustavoars@kernel.org> To: Steve French <sfrench@samba.org>, Paulo Alcantara <pc@manguebit.com>, Ronnie Sahlberg <lsahlber@redhat.com>, Shyam Prasad N <sprasad@microsoft.com>, Tom Talpey <tom@talpey.com>, Christian Brauner <brauner@kernel.org> Cc: linux-cifs@vger.kernel.org, samba-technical@lists.samba.org, linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" <gustavoars@kernel.org>, linux-hardening@vger.kernel.org Subject: [PATCH] smb: client: Fix -Wstringop-overflow issues Message-ID: <ZK3h3+dHBGONHt+S@work> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Precedence: bulk
Series	smb: client: Fix -Wstringop-overflow issues \| smb: client: Fix -Wstringop-overflow issues

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 */