On 18/05/2020 12:29, Alexandru Elisei wrote:
> Hi,
>
> On 5/17/20 4:28 PM, Andre Przywara wrote:
>> GCC 10.1 generates a warning in net/ip/csum.c about exceeding a buffer
>> limit in a memcpy operation:
>> ------------------
>> In function ‘memcpy’,
>> inlined from ‘uip_csum_udp’ at net/uip/csum.c:58:3:
>> /usr/include/aarch64-linux-gnu/bits/string_fortified.h:34:10: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>> 34 | return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from net/uip/csum.c:1:
>> net/uip/csum.c: In function ‘uip_csum_udp’:
>> include/kvm/uip.h:132:6: note: at offset 0 to object ‘sport’ with size 2 declared here
>> 132 | u16 sport;
>> ------------------
>
> When I apply this patch, I get unrecognized characters:
>
> In function <E2><80><98>memcpy<E2><80><99>,
> inlined from <E2><80><98>uip_csum_udp<E2><80><99> at net/uip/csum.c:58:3:
> /usr/include/aarch64-linux-gnu/bits/string_fortified.h:34:10: error: writing 1
> byte into a region of size 0 [-Werror=stringop-overflow=]
> 34 | return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from net/uip/csum.c:1:
> net/uip/csum.c: In function <E2><80><98>uip_csum_udp<E2><80><99>:
> include/kvm/uip.h:132:6: note: at offset 0 to object
> <E2><80><98>sport<E2><80><99> with size 2 declared here
> 132 | u16 sport;
>
> I looked at the patch source, and they're there also
Mmh, I see that GCC uses UTF-8 to render fancy ticks - for whatever
reason ;-) Removed them to avoid issues.
But where did you see those broken characters? Not sure if a non-UTF-8
capable terminal is still considered a thing in 2020?
I mean, even my Slackware is not ISO-8859-15 anymore ;-)
Fixed the rest and sent a v2.
Cheers,
Andre
>>
>> This warning originates from the code taking the address of the "sport"
>> member, then using that with some pointer arithmetic in a memcpy call.
>> GCC now sees that the object is only a u16, so copying 12 bytes into it
>> cannot be any good.
>> It's somewhat debatable whether this is a legitimate warning, as there
>> is enough storage at that place, and we knowingly use the struct and
>> its variabled-sized member at the end.
>>
>> However we can also rewrite the code, to not abuse the "&" operation of
>> some *member*, but take the address of the struct itself.
>> This makes the code less dodgy, and indeed appeases GCC 10.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> Reported-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>
> I've tested the patch and the compile errors have gone away for x86 and arm64.
> Tested virtio-net on both architectures and it works just like before:
>
> Tested-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>
>> ---
>> net/uip/csum.c | 26 ++++++++++++--------------
>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/net/uip/csum.c b/net/uip/csum.c
>> index 7ca8bada..607c9f1c 100644
>> --- a/net/uip/csum.c
>> +++ b/net/uip/csum.c
>> @@ -37,7 +37,7 @@ u16 uip_csum_udp(struct uip_udp *udp)
>> struct uip_pseudo_hdr hdr;
>> struct uip_ip *ip;
>> int udp_len;
>> - u8 *pad;
>> + u8 *udp_hdr = (u8*)udp + offsetof(struct uip_udp, sport);
>
> Super minor nitpick: there should be a space between u8 and *.
>
>>
>> ip = &udp->ip;
>>
>> @@ -50,13 +50,12 @@ u16 uip_csum_udp(struct uip_udp *udp)
>> udp_len = uip_udp_len(udp);
>>
>> if (udp_len % 2) {
>> - pad = (u8 *)&udp->sport + udp_len;
>> - *pad = 0;
>> - memcpy((u8 *)&udp->sport + udp_len + 1, &hdr, sizeof(hdr));
>> - return uip_csum(0, (u8 *)&udp->sport, udp_len + 1 + sizeof(hdr));
>> + udp_hdr[udp_len] = 0; /* zero padding */
>> + memcpy(udp_hdr + udp_len + 1, &hdr, sizeof(hdr));
>> + return uip_csum(0, udp_hdr, udp_len + 1 + sizeof(hdr));
>> } else {
>> - memcpy((u8 *)&udp->sport + udp_len, &hdr, sizeof(hdr));
>> - return uip_csum(0, (u8 *)&udp->sport, udp_len + sizeof(hdr));
>> + memcpy(udp_hdr + udp_len, &hdr, sizeof(hdr));
>> + return uip_csum(0, udp_hdr, udp_len + sizeof(hdr));
>> }
>>
>> }
>> @@ -66,7 +65,7 @@ u16 uip_csum_tcp(struct uip_tcp *tcp)
>> struct uip_pseudo_hdr hdr;
>> struct uip_ip *ip;
>> u16 tcp_len;
>> - u8 *pad;
>> + u8 *tcp_hdr = (u8*)tcp + offsetof(struct uip_tcp, sport);
>>
>> ip = &tcp->ip;
>> tcp_len = ntohs(ip->len) - uip_ip_hdrlen(ip);
>> @@ -81,12 +80,11 @@ u16 uip_csum_tcp(struct uip_tcp *tcp)
>> pr_warning("tcp_len(%d) is too large", tcp_len);
>>
>> if (tcp_len % 2) {
>> - pad = (u8 *)&tcp->sport + tcp_len;
>> - *pad = 0;
>> - memcpy((u8 *)&tcp->sport + tcp_len + 1, &hdr, sizeof(hdr));
>> - return uip_csum(0, (u8 *)&tcp->sport, tcp_len + 1 + sizeof(hdr));
>> + tcp_hdr[tcp_len] = 0; /* zero padding */
>> + memcpy(tcp_hdr + tcp_len + 1, &hdr, sizeof(hdr));
>> + return uip_csum(0, tcp_hdr, tcp_len + 1 + sizeof(hdr));
>> } else {
>> - memcpy((u8 *)&tcp->sport + tcp_len, &hdr, sizeof(hdr));
>> - return uip_csum(0, (u8 *)&tcp->sport, tcp_len + sizeof(hdr));
>> + memcpy(tcp_hdr + tcp_len, &hdr, sizeof(hdr));
>> + return uip_csum(0, tcp_hdr, tcp_len + sizeof(hdr));
>> }
>> }
>
> The patch looks functionally identical to the original version, and slightly
> easier to understand:
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
>
