| Message ID | 20230926071524.3638706-1-giulio.benetti@benettiengineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc-utils: fix potential overflow | expand |
> With fortify enabled gcc throws following error: > from mmc_cmds.c:20: > In function ‘read’, > inlined from ‘do_rpmb_write_key’ at mmc_cmds.c:2233:8: > /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017e > bb7db7/output/host/mipsel-buildroot-linux- > gnu/sysroot/usr/include/bits/unistd.h:38:10: error: ‘__read_alias’ writing > 228 or more bytes into a region of size 32 overflows the destination [- > Werror=stringop-overflow=] > 38 | return __glibc_fortify (read, __nbytes, sizeof (char), > | ^~~~~~~~~~~~~~~ > mmc_cmds.c: In function ‘do_rpmb_write_key’: > mmc_cmds.c:2087:19: note: destination object ‘key_mac’ of size 32 > 2087 | u_int8_t key_mac[32]; > | ^~~~~~~ > /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017e > bb7db7/output/host/mipsel-buildroot-linux- > gnu/sysroot/usr/include/bits/unistd.h:26:16: note: in a call to function > ‘__read_alias’ declared with attribute ‘access (write_only, 2, 3)’ > 26 | extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf, > | ^~~~~~~~~~ > > read() could potentially return more than nbyte so let's check for ret < nbyte. No it can't because that would mean buffer overflow. See https://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html : " this number shall never be greater than nbyte." So this seems like a bogus warning to me. Thanks, Avri > > Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com> > --- > mmc_cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mmc_cmds.c b/mmc_cmds.c > index 10d063d..ae7b876 100644 > --- a/mmc_cmds.c > +++ b/mmc_cmds.c > @@ -2065,7 +2065,7 @@ int do_sanitize(int nargs, char **argv) > } \ > else if (r > 0) \ > ret += r; \ > - } while (r != 0 && (size_t)ret != nbyte); \ > + } while (r != 0 && (size_t)ret < nbyte); \ > \ > ret; \ > }) > -- > 2.34.1
Hello Avri, On 26/09/23 11:54, Avri Altman wrote: >> With fortify enabled gcc throws following error: >> from mmc_cmds.c:20: >> In function ‘read’, >> inlined from ‘do_rpmb_write_key’ at mmc_cmds.c:2233:8: >> /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017e >> bb7db7/output/host/mipsel-buildroot-linux- >> gnu/sysroot/usr/include/bits/unistd.h:38:10: error: ‘__read_alias’ writing >> 228 or more bytes into a region of size 32 overflows the destination [- >> Werror=stringop-overflow=] >> 38 | return __glibc_fortify (read, __nbytes, sizeof (char), >> | ^~~~~~~~~~~~~~~ >> mmc_cmds.c: In function ‘do_rpmb_write_key’: >> mmc_cmds.c:2087:19: note: destination object ‘key_mac’ of size 32 >> 2087 | u_int8_t key_mac[32]; >> | ^~~~~~~ >> /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017e >> bb7db7/output/host/mipsel-buildroot-linux- >> gnu/sysroot/usr/include/bits/unistd.h:26:16: note: in a call to function >> ‘__read_alias’ declared with attribute ‘access (write_only, 2, 3)’ >> 26 | extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf, >> | ^~~~~~~~~~ >> >> read() could potentially return more than nbyte so let's check for ret < nbyte. > No it can't because that would mean buffer overflow. > See https://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html : > " this number shall never be greater than nbyte." > > So this seems like a bogus warning to me. Yes, you're right, and after doing some test this error appear with _FORTIFY_SOURCE=3 but it doesn't with _FORTIFY_SOURCE=2 or 1. So yes. I'm going to reword like: ``` Building with _FORTIFY_SOURCE=3 results in: from mmc_cmds.c:20: In function ‘read’, inlined from ‘do_rpmb_write_key’ at mmc_cmds.c:2233:8: /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017e bb7db7/output/host/mipsel-buildroot-linux- gnu/sysroot/usr/include/bits/unistd.h:38:10: error: ‘__read_alias’ writing 228 or more bytes into a region of size 32 overflows the destination [- Werror=stringop-overflow=] 38 | return __glibc_fortify (read, __nbytes, sizeof (char), | ^~~~~~~~~~~~~~~ mmc_cmds.c: In function ‘do_rpmb_write_key’: mmc_cmds.c:2087:19: note: destination object ‘key_mac’ of size 32 2087 | u_int8_t key_mac[32]; | ^~~~~~~ /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017e bb7db7/output/host/mipsel-buildroot-linux- gnu/sysroot/usr/include/bits/unistd.h:26:16: note: in a call to function ‘__read_alias’ declared with attribute ‘access (write_only, 2, 3)’ 26 | extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf, | ^~~~~~~~~~ To work around this let's check if the return of read() is lower than the nbyte requested instead of not equal. ``` Does it look good for you? Best regards
diff --git a/mmc_cmds.c b/mmc_cmds.c index 10d063d..ae7b876 100644 --- a/mmc_cmds.c +++ b/mmc_cmds.c @@ -2065,7 +2065,7 @@ int do_sanitize(int nargs, char **argv) } \ else if (r > 0) \ ret += r; \ - } while (r != 0 && (size_t)ret != nbyte); \ + } while (r != 0 && (size_t)ret < nbyte); \ \ ret; \ })
With fortify enabled gcc throws following error: from mmc_cmds.c:20: In function ‘read’, inlined from ‘do_rpmb_write_key’ at mmc_cmds.c:2233:8: /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017ebb7db7/output/host/mipsel-buildroot-linux-gnu/sysroot/usr/include/bits/unistd.h:38:10: error: ‘__read_alias’ writing 228 or more bytes into a region of size 32 overflows the destination [-Werror=stringop-overflow=] 38 | return __glibc_fortify (read, __nbytes, sizeof (char), | ^~~~~~~~~~~~~~~ mmc_cmds.c: In function ‘do_rpmb_write_key’: mmc_cmds.c:2087:19: note: destination object ‘key_mac’ of size 32 2087 | u_int8_t key_mac[32]; | ^~~~~~~ /home/giuliobenetti/br_reproduce/a53922c5db3e605a5e81e53c034f45017ebb7db7/output/host/mipsel-buildroot-linux-gnu/sysroot/usr/include/bits/unistd.h:26:16: note: in a call to function ‘__read_alias’ declared with attribute ‘access (write_only, 2, 3)’ 26 | extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf, | ^~~~~~~~~~ read() could potentially return more than nbyte so let's check for ret < nbyte. Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com> --- mmc_cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)