| Message ID | 20230616090749.2646749-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: avoid field overflow warning | expand |
2023-06-16 18:07 GMT+09:00, Arnd Bergmann <arnd@kernel.org>: > From: Arnd Bergmann <arnd@arndb.de> Hi Arnd, > > clang warns about a possible field overflow in a memcpy: > > In file included from fs/smb/server/smb_common.c:7: > include/linux/fortify-string.h:583:4: error: call to > '__write_overflow_field' declared with 'warning' attribute: detected write > beyond size of field (1st parameter); maybe use struct_group()? > [-Werror,-Wattribute-warning] > __write_overflow_field(p_size_field, size); > > It appears to interpret the "&out[baselen + 4]" as referring to a single > byte of the character array, while the equivalen "out + baselen + 4" is > seen as an offset into the array. > > I don't see that kind of warning elsewhere, so just go with the simple > rework. > > Fixes: e2f34481b24db ("cifsd: add server-side procedures for SMB3") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/smb/server/smb_common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c > index a7e81067bc991..e3273fa640b07 100644 > --- a/fs/smb/server/smb_common.c > +++ b/fs/smb/server/smb_common.c > @@ -536,7 +536,7 @@ int ksmbd_extract_shortname(struct ksmbd_conn *conn, > const char *longname, > out[baselen + 3] = PERIOD; > > if (dot_present) > - memcpy(&out[baselen + 4], extension, 4); > + memcpy(out + baselen + 4, extension, 1); Is there any reason to change copy bytes from 4 bytes to 1 byte? Thanks! > else > out[baselen + 4] = '\0'; > smbConvertToUTF16((__le16 *)shortname, out, PATH_MAX, > -- > 2.39.2 > >
On Fri, Jun 16, 2023, at 16:40, Namjae Jeon wrote: > 2023-06-16 18:07 GMT+09:00, Arnd Bergmann <arnd@kernel.org>: >> From: Arnd Bergmann <arnd@arndb.de> >> >> clang warns about a possible field overflow in a memcpy: >> >> In file included from fs/smb/server/smb_common.c:7: >> include/linux/fortify-string.h:583:4: error: call to >> '__write_overflow_field' declared with 'warning' attribute: detected write >> beyond size of field (1st parameter); maybe use struct_group()? >> [-Werror,-Wattribute-warning] >> __write_overflow_field(p_size_field, size); >> >> It appears to interpret the "&out[baselen + 4]" as referring to a single >> byte of the character array, while the equivalen "out + baselen + 4" is >> seen as an offset into the array. >> >> I don't see that kind of warning elsewhere, so just go with the simple >> rework. >> >> Fixes: e2f34481b24db ("cifsd: add server-side procedures for SMB3") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> fs/smb/server/smb_common.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c >> index a7e81067bc991..e3273fa640b07 100644 >> --- a/fs/smb/server/smb_common.c >> +++ b/fs/smb/server/smb_common.c >> @@ -536,7 +536,7 @@ int ksmbd_extract_shortname(struct ksmbd_conn *conn, >> const char *longname, >> out[baselen + 3] = PERIOD; >> >> if (dot_present) >> - memcpy(&out[baselen + 4], extension, 4); >> + memcpy(out + baselen + 4, extension, 1); > Is there any reason to change copy bytes from 4 bytes to 1 byte? > No, that was an accident, this patch is wrong. I have to revisit this one and check if my change actually still works after I change it back to the correct length. Arnd
2023-06-16 23:42 GMT+09:00, Arnd Bergmann <arnd@arndb.de>: > On Fri, Jun 16, 2023, at 16:40, Namjae Jeon wrote: >> 2023-06-16 18:07 GMT+09:00, Arnd Bergmann <arnd@kernel.org>: >>> From: Arnd Bergmann <arnd@arndb.de> >>> >>> clang warns about a possible field overflow in a memcpy: >>> >>> In file included from fs/smb/server/smb_common.c:7: >>> include/linux/fortify-string.h:583:4: error: call to >>> '__write_overflow_field' declared with 'warning' attribute: detected >>> write >>> beyond size of field (1st parameter); maybe use struct_group()? >>> [-Werror,-Wattribute-warning] >>> __write_overflow_field(p_size_field, size); >>> >>> It appears to interpret the "&out[baselen + 4]" as referring to a single >>> byte of the character array, while the equivalen "out + baselen + 4" is >>> seen as an offset into the array. >>> >>> I don't see that kind of warning elsewhere, so just go with the simple >>> rework. >>> >>> Fixes: e2f34481b24db ("cifsd: add server-side procedures for SMB3") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> --- >>> fs/smb/server/smb_common.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c >>> index a7e81067bc991..e3273fa640b07 100644 >>> --- a/fs/smb/server/smb_common.c >>> +++ b/fs/smb/server/smb_common.c >>> @@ -536,7 +536,7 @@ int ksmbd_extract_shortname(struct ksmbd_conn *conn, >>> const char *longname, >>> out[baselen + 3] = PERIOD; >>> >>> if (dot_present) >>> - memcpy(&out[baselen + 4], extension, 4); >>> + memcpy(out + baselen + 4, extension, 1); >> Is there any reason to change copy bytes from 4 bytes to 1 byte? >> > > No, that was an accident, this patch is wrong. > > I have to revisit this one and check if my change actually still works > after I change it back to the correct length. Okay:) Thanks for your check! > > Arnd >
diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c index a7e81067bc991..e3273fa640b07 100644 --- a/fs/smb/server/smb_common.c +++ b/fs/smb/server/smb_common.c @@ -536,7 +536,7 @@ int ksmbd_extract_shortname(struct ksmbd_conn *conn, const char *longname, out[baselen + 3] = PERIOD; if (dot_present) - memcpy(&out[baselen + 4], extension, 4); + memcpy(out + baselen + 4, extension, 1); else out[baselen + 4] = '\0'; smbConvertToUTF16((__le16 *)shortname, out, PATH_MAX,