Re: net: use-after-free in recvmmsg

[Dmitry Vyukov]

On Fri, Jan 22, 2016 at 10:16 PM, Arnaldo Carvalho de Melo
<acme@xxxxxxxxxx> wrote:
> Em Fri, Jan 22, 2016 at 09:39:53PM +0100, Dmitry Vyukov escreveu:
>> While running syzkaller fuzzer I've hit the following use-after-free:
>
> <SNIP>
>
>> Call Trace:
>>  [<ffffffff8175ea0e>] __asan_report_load8_noabort+0x3e/0x40
>> mm/kasan/report.c:295
>>  [<ffffffff851cc31a>] __sys_recvmmsg+0x6fa/0x7f0 net/socket.c:2261
>>  [<     inline     >] SYSC_recvmmsg net/socket.c:2281
>>  [<ffffffff851cc57f>] SyS_recvmmsg+0x16f/0x180 net/socket.c:2270
>>  [<ffffffff86332bb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
>> arch/x86/entry/entry_64.S:185
>> ==================================================================
>>
>> I cannot reproduce it, but looking at __sys_recvmmsg code, it seems
>> that sock is not necessary live after fput_light:
>>
>> out_put:
>>     fput_light(sock->file, fput_needed);
>>
>>     if (err == 0)
>>         return datagrams;
>>
>>     if (datagrams != 0) {
>>         /*
>>          * We may return less entries than requested (vlen) if the
>>          * sock is non block and there aren't enough datagrams...
>>          */
>>         if (err != -EAGAIN) {
>>             /*
>>              * ... or  if recvmsg returns an error after we
>>              * received some datagrams, where we record the
>>              * error to return on the next call or if the
>>              * app asks about it using getsockopt(SO_ERROR).
>>              */
>>             sock->sk->sk_err = -err;
>>         }
>>
>>         return datagrams;
>>     }
>>
>>     return err;
>> }
>>
>> I am on commit 30f05309bde49295e02e45c7e615f73aa4e0ccc2 (Jan 20).
>> Seems to be added in commit a2e2725541fad72416326798c2d7fa4dafb7d337
>> (Oct 2009).
>
> Maybe this helps? Compile testing now...


I don't have a reliable reproducer, so can't test it per se.
I will integrate this patch tomorrow and restart fuzzer with it.


> diff --git a/net/socket.c b/net/socket.c
> index 91c2de6f5020..03e57ad7ec9f 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2240,31 +2240,31 @@ int __sys_recvmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
>                 cond_resched();
>         }
>
> -out_put:
> -       fput_light(sock->file, fput_needed);
> -
>         if (err == 0)
> -               return datagrams;
> +               goto out_put;
>
> -       if (datagrams != 0) {
> +       if (datagrams == 0) {
> +               datagrams = err;
> +               goto out_put;
> +       }
> +
> +       /*
> +        * We may return less entries than requested (vlen) if the
> +        * sock is non block and there aren't enough datagrams...
> +        */
> +       if (err != -EAGAIN) {
>                 /*
> -                * We may return less entries than requested (vlen) if the
> -                * sock is non block and there aren't enough datagrams...
> +                * ... or  if recvmsg returns an error after we
> +                * received some datagrams, where we record the
> +                * error to return on the next call or if the
> +                * app asks about it using getsockopt(SO_ERROR).
>                  */
> -               if (err != -EAGAIN) {
> -                       /*
> -                        * ... or  if recvmsg returns an error after we
> -                        * received some datagrams, where we record the
> -                        * error to return on the next call or if the
> -                        * app asks about it using getsockopt(SO_ERROR).
> -                        */
> -                       sock->sk->sk_err = -err;
> -               }
> -
> -               return datagrams;
> +               sock->sk->sk_err = -err;
>         }
> +out_put:
> +       fput_light(sock->file, fput_needed);
>
> -       return err;
> +       return datagrams;
>  }
>
>  SYSCALL_DEFINE5(recvmmsg, int, fd, struct mmsghdr __user *, mmsg,