Re: [PATCH] af_unix: fix holding spinlock in oob handling

[Shoaib Rao <rao.shoaib@oracle.com>]

Hi Eric,

Thanks for your review I will take care of the comments.

Shoaib

On 8/12/21 12:53 AM, Eric Dumazet wrote:
> On Thu, Aug 12, 2021 at 12:07 AM Rao Shoaib <Rao.Shoaib@oracle.com> wrote:
>> From: Rao Shoaib <rao.shoaib@oracle.com>
>>
>> syzkaller found that OOB code was holding spinlock
>> while calling a function in which it could sleep.
>>
>> Reported-by: syzbot+8760ca6c1ee783ac4abd@syzkaller.appspotmail.com
>>
>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>
>> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
>> ---
> Please do not add these empty lines.
>
> Fixes: ...
> Reported-by: ...
> Signed-off-by: ...
> Also you might take a look at queue_oob()
>
> 1)  Setting skb->len tp 1 should not be needed, skb_put() already does that
> 2) After unix_state_lock(other); we probably need to check status of
> the other socket.
> 3) Some skb_free() calls should have been consume_skb()
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index ec02e70a549b42f6c102253508c48426a13f7bc4..0c27e2976f9d234ca3bb131731375bc51a056846
> 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1908,7 +1908,6 @@ static int queue_oob(struct socket *sock, struct
> msghdr *msg, struct sock *other
>                  return err;
>
>          skb_put(skb, 1);
> -       skb->len = 1;
>          err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, 1);
>
>          if (err) {
> @@ -1917,11 +1916,17 @@ static int queue_oob(struct socket *sock,
> struct msghdr *msg, struct sock *other
>          }
>
>          unix_state_lock(other);
> +       if (sock_flag(other, SOCK_DEAD) ||
> +           (other->sk_shutdown & RCV_SHUTDOWN)) {
> +               unix_state_unlock(other);
> +               kfree_skb(skb);
> +               return -EPIPE;
> +       }
>          maybe_add_creds(skb, sock, other);
>          skb_get(skb);
>
>          if (ousk->oob_skb)
> -               kfree_skb(ousk->oob_skb);
> +               consume_skb(ousk->oob_skb);
>
>          ousk->oob_skb = skb;