slirp: tftp, copy sockaddr_size

[Qemu-devel] [PATCH] slirp: tftp, copy sockaddr_size

Posted by Marc-André Lureau 7 years, 1 month ago

ASAN detects an "unknown-crash" when running pxe-test:

/ppc64/pxe/spapr-vlan: =================================================================
==7143==ERROR: AddressSanitizer: unknown-crash on address 0x7f6dcd298d30 at pc 0x55e22218830d bp 0x7f6dcd2989e0 sp 0x7f6dcd2989d0
READ of size 128 at 0x7f6dcd298d30 thread T2
    #0 0x55e22218830c in tftp_session_allocate /home/elmarco/src/qq/slirp/tftp.c:73
    #1 0x55e22218a1f8 in tftp_handle_rrq /home/elmarco/src/qq/slirp/tftp.c:289
    #2 0x55e22218b54c in tftp_input /home/elmarco/src/qq/slirp/tftp.c:446
    #3 0x55e2221833fe in udp6_input /home/elmarco/src/qq/slirp/udp6.c:82
    #4 0x55e222137b17 in ip6_input /home/elmarco/src/qq/slirp/ip6_input.c:67

Address 0x7f6dcd298d30 is located in stack of thread T2 at offset 96 in frame
    #0 0x55e222182420 in udp6_input /home/elmarco/src/qq/slirp/udp6.c:13

  This frame has 3 object(s):
    [32, 48) '<unknown>'
    [96, 124) 'lhost' <== Memory access at offset 96 partially overflows this variable
    [160, 200) 'save_ip' <== Memory access at offset 96 partially underflows this variable

The sockaddr_storage pointer is the sockaddr_in6 lhost on the
stack. Copy only the source addr size.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/tftp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index 50e714807d..a9bc4bb1b6 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -70,7 +70,7 @@ static int tftp_session_allocate(Slirp *slirp, struct sockaddr_storage *srcsas,
 
  found:
   memset(spt, 0, sizeof(*spt));
-  spt->client_addr = *srcsas;
+  memcpy(&spt->client_addr, srcsas, sockaddr_size(srcsas));
   spt->fd = -1;
   spt->block_size = 512;
   spt->client_port = tp->udp.uh_sport;
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH] slirp: tftp, copy sockaddr_size

Posted by Samuel Thibault 7 years, 1 month ago

Marc-André Lureau, on jeu. 23 mars 2017 15:31:56 +0400, wrote:
> ASAN detects an "unknown-crash" when running pxe-test:
> 
> /ppc64/pxe/spapr-vlan: =================================================================
> ==7143==ERROR: AddressSanitizer: unknown-crash on address 0x7f6dcd298d30 at pc 0x55e22218830d bp 0x7f6dcd2989e0 sp 0x7f6dcd2989d0
> READ of size 128 at 0x7f6dcd298d30 thread T2
>     #0 0x55e22218830c in tftp_session_allocate /home/elmarco/src/qq/slirp/tftp.c:73
>     #1 0x55e22218a1f8 in tftp_handle_rrq /home/elmarco/src/qq/slirp/tftp.c:289
>     #2 0x55e22218b54c in tftp_input /home/elmarco/src/qq/slirp/tftp.c:446
>     #3 0x55e2221833fe in udp6_input /home/elmarco/src/qq/slirp/udp6.c:82
>     #4 0x55e222137b17 in ip6_input /home/elmarco/src/qq/slirp/ip6_input.c:67
> 
> Address 0x7f6dcd298d30 is located in stack of thread T2 at offset 96 in frame
>     #0 0x55e222182420 in udp6_input /home/elmarco/src/qq/slirp/udp6.c:13
> 
>   This frame has 3 object(s):
>     [32, 48) '<unknown>'
>     [96, 124) 'lhost' <== Memory access at offset 96 partially overflows this variable
>     [160, 200) 'save_ip' <== Memory access at offset 96 partially underflows this variable
> 
> The sockaddr_storage pointer is the sockaddr_in6 lhost on the
> stack. Copy only the source addr size.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

> ---
>  slirp/tftp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 50e714807d..a9bc4bb1b6 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -70,7 +70,7 @@ static int tftp_session_allocate(Slirp *slirp, struct sockaddr_storage *srcsas,
>  
>   found:
>    memset(spt, 0, sizeof(*spt));
> -  spt->client_addr = *srcsas;
> +  memcpy(&spt->client_addr, srcsas, sockaddr_size(srcsas));
>    spt->fd = -1;
>    spt->block_size = 512;
>    spt->client_port = tp->udp.uh_sport;
> -- 
> 2.12.0.191.gc5d8de91d
> 

-- 
Samuel
gawk; talk; nice; date; wine; grep; touch; unzip; strip;  \
touch; gasp; finger; gasp; lyx; gasp; latex; mount; fsck; \
more; yes; gasp; umount; make clean; make mrproper; sleep

Re: [Qemu-devel] [PATCH] slirp: tftp, copy sockaddr_size

Posted by Thomas Huth 7 years, 1 month ago

On 23.03.2017 12:31, Marc-André Lureau wrote:
> ASAN detects an "unknown-crash" when running pxe-test:
> 
> /ppc64/pxe/spapr-vlan: =================================================================
> ==7143==ERROR: AddressSanitizer: unknown-crash on address 0x7f6dcd298d30 at pc 0x55e22218830d bp 0x7f6dcd2989e0 sp 0x7f6dcd2989d0
> READ of size 128 at 0x7f6dcd298d30 thread T2
>     #0 0x55e22218830c in tftp_session_allocate /home/elmarco/src/qq/slirp/tftp.c:73
>     #1 0x55e22218a1f8 in tftp_handle_rrq /home/elmarco/src/qq/slirp/tftp.c:289
>     #2 0x55e22218b54c in tftp_input /home/elmarco/src/qq/slirp/tftp.c:446
>     #3 0x55e2221833fe in udp6_input /home/elmarco/src/qq/slirp/udp6.c:82
>     #4 0x55e222137b17 in ip6_input /home/elmarco/src/qq/slirp/ip6_input.c:67
> 
> Address 0x7f6dcd298d30 is located in stack of thread T2 at offset 96 in frame
>     #0 0x55e222182420 in udp6_input /home/elmarco/src/qq/slirp/udp6.c:13
> 
>   This frame has 3 object(s):
>     [32, 48) '<unknown>'
>     [96, 124) 'lhost' <== Memory access at offset 96 partially overflows this variable
>     [160, 200) 'save_ip' <== Memory access at offset 96 partially underflows this variable
> 
> The sockaddr_storage pointer is the sockaddr_in6 lhost on the
> stack. Copy only the source addr size.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  slirp/tftp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 50e714807d..a9bc4bb1b6 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -70,7 +70,7 @@ static int tftp_session_allocate(Slirp *slirp, struct sockaddr_storage *srcsas,
>  
>   found:
>    memset(spt, 0, sizeof(*spt));
> -  spt->client_addr = *srcsas;
> +  memcpy(&spt->client_addr, srcsas, sockaddr_size(srcsas));
>    spt->fd = -1;
>    spt->block_size = 512;
>    spt->client_port = tp->udp.uh_sport;
> 

Makes sense.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] slirp: tftp, copy sockaddr_size

Posted by Philippe Mathieu-Daudé 7 years ago

On 03/23/2017 08:31 AM, Marc-André Lureau wrote:
> ASAN detects an "unknown-crash" when running pxe-test:
>
> /ppc64/pxe/spapr-vlan: =================================================================
> ==7143==ERROR: AddressSanitizer: unknown-crash on address 0x7f6dcd298d30 at pc 0x55e22218830d bp 0x7f6dcd2989e0 sp 0x7f6dcd2989d0
> READ of size 128 at 0x7f6dcd298d30 thread T2
>     #0 0x55e22218830c in tftp_session_allocate /home/elmarco/src/qq/slirp/tftp.c:73
>     #1 0x55e22218a1f8 in tftp_handle_rrq /home/elmarco/src/qq/slirp/tftp.c:289
>     #2 0x55e22218b54c in tftp_input /home/elmarco/src/qq/slirp/tftp.c:446
>     #3 0x55e2221833fe in udp6_input /home/elmarco/src/qq/slirp/udp6.c:82

Interesting bug, udp6.c seems to have been started as a copy of udp.c, 
which uses:

	struct sockaddr_storage lhost;
	struct sockaddr_in *lhost4;

but in udp6.c this was changed to:

     struct sockaddr_in6 lhost;

tftp_session_allocate() behavior was fine with IPv4 but dual-stack broke it.

Your fix uses sockaddr_size() which is the cleaner/safer way to do it 
but I'm wondering how to avoid such dual-stack sockaddr issues before 
runtime ASAN spotting.

One way can be to use 'union slirp_sockaddr' recently extracted by Dave 
Gilbert (see http://patchwork.ozlabs.org/patch/746605), it uses more 
stack but this avoid partial underflows.

>     #4 0x55e222137b17 in ip6_input /home/elmarco/src/qq/slirp/ip6_input.c:67
>
> Address 0x7f6dcd298d30 is located in stack of thread T2 at offset 96 in frame
>     #0 0x55e222182420 in udp6_input /home/elmarco/src/qq/slirp/udp6.c:13
>
>   This frame has 3 object(s):
>     [32, 48) '<unknown>'
>     [96, 124) 'lhost' <== Memory access at offset 96 partially overflows this variable
>     [160, 200) 'save_ip' <== Memory access at offset 96 partially underflows this variable
>
> The sockaddr_storage pointer is the sockaddr_in6 lhost on the
> stack. Copy only the source addr size.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  slirp/tftp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 50e714807d..a9bc4bb1b6 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -70,7 +70,7 @@ static int tftp_session_allocate(Slirp *slirp, struct sockaddr_storage *srcsas,
>
>   found:
>    memset(spt, 0, sizeof(*spt));
> -  spt->client_addr = *srcsas;
> +  memcpy(&spt->client_addr, srcsas, sockaddr_size(srcsas));
>    spt->fd = -1;
>    spt->block_size = 512;
>    spt->client_port = tp->udp.uh_sport;
>

Re: [Qemu-devel] [PATCH] slirp: tftp, copy sockaddr_size

Posted by Michael Tokarev 7 years ago

23.03.2017 14:31, Marc-André Lureau wrote:
..
> The sockaddr_storage pointer is the sockaddr_in6 lhost on the
> stack. Copy only the source addr size.

Applied to -trivial, thanks!

/mjt