[Qemu-devel] Fix for accept

[Qemu-devel] Fix for accept

[Pablo Virolainen]

[-- Attachment #1: Type: text/plain, Size: 174 bytes --]


Following code crashes qemu user emulation.

#include <sys/types.h>
#include <sys/socket.h>

int main() {
        accept(0,NULL,NULL);
        return 0;
}

Pablo Virolainen

[-- Attachment #2: accept.patch --]
[-- Type: text/x-patch, Size: 1086 bytes --]

Index: linux-user/syscall.c
===================================================================
RCS file: /sources/qemu/qemu/linux-user/syscall.c,v
retrieving revision 1.75
diff -u -r1.75 syscall.c
--- linux-user/syscall.c	27 Jun 2006 21:08:10 -0000	1.75
+++ linux-user/syscall.c	13 Jul 2006 10:18:57 -0000
@@ -878,9 +878,20 @@
             int sockfd = tgetl(vptr);
             target_ulong target_addr = tgetl(vptr + n);
             target_ulong target_addrlen = tgetl(vptr + 2 * n);
-            socklen_t addrlen = tget32(target_addrlen);
-            void *addr = alloca(addrlen);
-
+            socklen_t addrlen=0;
+	    /* Just to get rid of compiler warnings */
+	    ulong addrt=0;
+            void *addr;
+	    
+	    get_user(addrlen,&target_addrlen);
+	    get_user(addrt,&target_addr);
+	    
+	    if (addrt!=0) {
+	        addr = alloca(addrlen);
+	    } else {
+	        addr = NULL;
+	    }
+	    
             ret = get_errno(accept(sockfd, addr, &addrlen));
             if (!is_error(ret)) {
                 host_to_target_sockaddr(target_addr, addr, addrlen);

Re: [Qemu-devel] Fix for accept

[Fabrice Bellard]

Hi,

OK for the bug report, but the fix is not correct because the problem is 
generic. [get|put]_user() and the other functions should be used 
everywhere to communicate with the "user" space and to generate the 
-EFAULT error if the address is not correct. For that purpose the host 
signal SIGSEGV can be catched and asm macros can be used to see if it is 
an expected seg fault (in this case [get|put]_user must return an error 
code) or if it is a QEMU bug. Note that exactly the same system is used 
inside the Linux kernel and I don't think it is necessary to invent 
something else.

Regards,

Fabrice.

Pablo Virolainen wrote:
> Following code crashes qemu user emulation.
> 
> #include <sys/types.h>
> #include <sys/socket.h>
> 
> int main() {
>         accept(0,NULL,NULL);
>         return 0;
> }
> 
> Pablo Virolainen
> 
> 
> ------------------------------------------------------------------------
> 
> Index: linux-user/syscall.c
> ===================================================================
> RCS file: /sources/qemu/qemu/linux-user/syscall.c,v
> retrieving revision 1.75
> diff -u -r1.75 syscall.c
> --- linux-user/syscall.c	27 Jun 2006 21:08:10 -0000	1.75
> +++ linux-user/syscall.c	13 Jul 2006 10:18:57 -0000
> @@ -878,9 +878,20 @@
>              int sockfd = tgetl(vptr);
>              target_ulong target_addr = tgetl(vptr + n);
>              target_ulong target_addrlen = tgetl(vptr + 2 * n);
> -            socklen_t addrlen = tget32(target_addrlen);
> -            void *addr = alloca(addrlen);
> -
> +            socklen_t addrlen=0;
> +	    /* Just to get rid of compiler warnings */
> +	    ulong addrt=0;
> +            void *addr;
> +	    
> +	    get_user(addrlen,&target_addrlen);
> +	    get_user(addrt,&target_addr);
> +	    
> +	    if (addrt!=0) {
> +	        addr = alloca(addrlen);
> +	    } else {
> +	        addr = NULL;
> +	    }
> +	    
>              ret = get_errno(accept(sockfd, addr, &addrlen));
>              if (!is_error(ret)) {
>                  host_to_target_sockaddr(target_addr, addr, addrlen);
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel

Re: [Qemu-devel] Fix for accept

[Pablo Virolainen]

Fabrice Bellard wrote:

> Hi,
>
> OK for the bug report, but the fix is not correct because the problem
> is generic. [get|put]_user() and the other functions should be used
> everywhere to communicate with the "user" space and to generate the
> -EFAULT error if the address is not correct. For that purpose the host
> signal SIGSEGV can be catched and asm macros can be used to see if it
> is an expected seg fault (in this case [get|put]_user must return an
> error code) or if it is a QEMU bug. Note that exactly the same system
> is used inside the Linux kernel and I don't think it is necessary to
> invent something else.
>
> Regards,
>
> Fabrice.

Hello,

So I should write something like following instead?

        if (!get_user(addrlen,&target_addrlen)) {
          return -EFAULT
        }

The code seems to assume target_sockaddr == sockaddr, so why allocate
temporary buffer and then do copying?
One could implement SOCKOP_[accept|getsockname|getpeername] with same
code. Perhaps something like

static long do_socketcall_helper(target_ulong vptr,
                 int (*func)(int,struct sockaddr*,socklen_t*))
{
    const int n = sizeof(target_ulong);
    if (access_ok(VERIFY_READ,vptr,n*3)) {
        int ret;
        int sockfd=tgetl(vptr);
        target_ulong target_addr = tgetl(vptr + n);
        target_ulong target_addrlen = tgetl(vptr + 2 * n);
        struct sockaddr *addr=(struct sockaddr *)target_addr;
        socklen_t addrlen;
  
        if (!get_user(addrlen,&target_addrlen) ||
!acces_ok(VERIFY_WRITE,target_addrlen,4)) {
              return -EFAULT;
        }
   
        ret=get_errno(func(sockfd, addr, &addrlen));
        if (!is_error(ret)) {
            host_to_target_sockaddr(target_addr, addr, addrlen);
            tput32(target_addrlen, addrlen);
       }
        return ret;
    }
    return -EFAULT;
}

....

    case SOCKOP_accept:
        ret = do_socketcall_helper(vptr,accept);
        break;
    case SOCKOP_getsockname:
        ret = do_socketcall_helper(vptr,getsockname);
        break;
    case SOCKOP_getpeername:
        ret = do_socketcall_helper(vptr,getpeername);
        break;


Pablo

Re: [Qemu-devel] Fix for accept

[Fabrice Bellard]

Pablo Virolainen wrote:

> So I should write something like following instead?
> 
>         if (!get_user(addrlen,&target_addrlen)) {
>           return -EFAULT
>         }

Yes.

> The code seems to assume target_sockaddr == sockaddr, so why allocate
> temporary buffer and then do copying?

If the assumption "target_sockaddr == sockaddr" is made, then it is a bug.

> One could implement SOCKOP_[accept|getsockname|getpeername] with same
> code.
 > [...]

You can use common code, but I think it is clearer to have the 
do_accept, do_getsockname and do_getpeername helpers.

Fabrice.