| Message ID | 20200517152849.204717-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvmtool] net: uip: Fix GCC 10 warning about checksum calculation | expand |
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 > > 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@arm.com> > Reported-by: Alexandru Elisei <alexandru.elisei@arm.com> 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@arm.com> > --- > 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@arm.com>
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@arm.com> >> Reported-by: Alexandru Elisei <alexandru.elisei@arm.com> > > 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@arm.com> > >> --- >> 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@arm.com> >
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); 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)); } }
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; ------------------ 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@arm.com> Reported-by: Alexandru Elisei <alexandru.elisei@arm.com> --- net/uip/csum.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)