kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH kvmtool] net: uip: Fix GCC 10 warning about checksum calculation
@ 2020-05-17 15:28 Andre Przywara
  2020-05-18 11:29 ` Alexandru Elisei
  0 siblings, 1 reply; 3+ messages in thread
From: Andre Przywara @ 2020-05-17 15:28 UTC (permalink / raw)
  To: Will Deacon, Julien Thierry; +Cc: Alexandru Elisei, kvm

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(-)

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));
 	}
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH kvmtool] net: uip: Fix GCC 10 warning about checksum calculation
  2020-05-17 15:28 [PATCH kvmtool] net: uip: Fix GCC 10 warning about checksum calculation Andre Przywara
@ 2020-05-18 11:29 ` Alexandru Elisei
  2020-05-18 13:01   ` André Przywara
  0 siblings, 1 reply; 3+ messages in thread
From: Alexandru Elisei @ 2020-05-18 11:29 UTC (permalink / raw)
  To: Andre Przywara, Will Deacon, Julien Thierry; +Cc: kvm

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>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH kvmtool] net: uip: Fix GCC 10 warning about checksum calculation
  2020-05-18 11:29 ` Alexandru Elisei
@ 2020-05-18 13:01   ` André Przywara
  0 siblings, 0 replies; 3+ messages in thread
From: André Przywara @ 2020-05-18 13:01 UTC (permalink / raw)
  To: Alexandru Elisei, Will Deacon, Julien Thierry; +Cc: kvm

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>
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-05-18 13:02 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-17 15:28 [PATCH kvmtool] net: uip: Fix GCC 10 warning about checksum calculation Andre Przywara
2020-05-18 11:29 ` Alexandru Elisei
2020-05-18 13:01   ` André Przywara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).