| Message ID | ZK3h3+dHBGONHt+S@work |
|---|---|
| State | New |
| Headers | show |
| Series | smb: client: Fix -Wstringop-overflow issues | expand |
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 >
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 --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 */
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(-)