MIME-Version: 1.0
References: <20230131-tuntap-sk-uid-v2-0-29ec15592813@diag.uniroma1.it> <20230131-tuntap-sk-uid-v2-1-29ec15592813@diag.uniroma1.it>
In-Reply-To: <20230131-tuntap-sk-uid-v2-1-29ec15592813@diag.uniroma1.it>
From:   Eric Dumazet <edumazet@google.com>
Date:   Fri, 3 Feb 2023 15:58:23 +0100
Message-ID: <CANn89i+QMipmfOywjAX2jqZGs80zorE6yKFOsi9rXQbToZLbhQ@mail.gmail.com>
Subject: Re: [PATCH net-next v2 1/2] tun: tun_chr_open(): correctly initialize
 socket uid
To:     Pietro Borrello <borrello@diag.uniroma1.it>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Lorenzo Colitti <lorenzo@google.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>, Jakob Koschel <jkl820.git@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 3, 2023 at 3:30 PM Pietro Borrello
<borrello@diag.uniroma1.it> wrote:
>
> sock_init_data() assumes that the `struct socket` passed in input is
> contained in a `struct socket_alloc` allocated with sock_alloc().
> However, tun_chr_open() passes a `struct socket` embedded in a `struct
> tun_file` allocated with sk_alloc().
> This causes a type confusion when issuing a container_of() with
> SOCK_INODE() in sock_init_data() which results in assigning a wrong
> sk_uid to the `struct sock` in input.
> On default configuration, the type confused field overlaps with the
> high 4 bytes of `struct tun_struct __rcu *tun` of `struct tun_file`,
> NULL at the time of call, which makes the uid of all tun sockets 0,
> i.e., the root one.  Fix the assignment by overriding it with the
> correct uid.
>
> Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
>  drivers/net/tun.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index a7d17c680f4a..ccffbc439c95 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3450,6 +3450,11 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>
>         sock_init_data(&tfile->socket, &tfile->sk);
>
> +       /* Assign sk_uid from the inode argument, since tfile->socket
> +        * passed to sock_init_data() has no corresponding inode
> +        */
> +       tfile->sk.sk_uid = inode->i_uid;
> +
>         tfile->sk.sk_write_space = tun_sock_write_space;
>         tfile->sk.sk_sndbuf = INT_MAX;
>


This seems very fragile...
"struct inode" could be made bigger, and __randomize_layout could move i_uid
at the end of it.

KASAN could then detect an out-of-bound access in sock_init_data()

I would rather add a wrapper like this [1], then change tun/tap to use
sock_init_data_uid()

diff --git a/include/net/sock.h b/include/net/sock.h
index 9e464f6409a7175cef5f8ec22e70cade19df5e60..a7e1396d1b778c8a7ed664d149bd6c82cf2ae422
100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1946,6 +1946,8 @@ void sk_common_release(struct sock *sk);
  */

 /* Initialise core socket variables */
+void sock_init_data_uid(struct socket *sock, struct sock *sk, kuid_t uid);
+
 void sock_init_data(struct socket *sock, struct sock *sk);

 /*
diff --git a/net/core/sock.c b/net/core/sock.c
index a3ba0358c77c0e44db1cfbaeb420f8b80ad7cf98..d811cd0d204f37b1791ae94ccfe95114a2286caf
100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3357,7 +3357,7 @@ void sk_stop_timer_sync(struct sock *sk, struct
timer_list *timer)
 }
 EXPORT_SYMBOL(sk_stop_timer_sync);

-void sock_init_data(struct socket *sock, struct sock *sk)
+void sock_init_data_uid(struct socket *sock, struct sock *sk, kuid_t uid)
 {
        sk_init_common(sk);
        sk->sk_send_head        =       NULL;
@@ -3376,11 +3376,10 @@ void sock_init_data(struct socket *sock,
struct sock *sk)
                sk->sk_type     =       sock->type;
                RCU_INIT_POINTER(sk->sk_wq, &sock->wq);
                sock->sk        =       sk;
-               sk->sk_uid      =       SOCK_INODE(sock)->i_uid;
        } else {
                RCU_INIT_POINTER(sk->sk_wq, NULL);
-               sk->sk_uid      =       make_kuid(sock_net(sk)->user_ns, 0);
        }
+       sk->sk_uid      =       uid;

        rwlock_init(&sk->sk_callback_lock);
        if (sk->sk_kern_sock)
@@ -3439,6 +3438,16 @@ void sock_init_data(struct socket *sock, struct sock *sk)
        refcount_set(&sk->sk_refcnt, 1);
        atomic_set(&sk->sk_drops, 0);
 }
+EXPORT_SYMBOL(sock_init_data_uid);
+
+void sock_init_data(struct socket *sock, struct sock *sk)
+{
+       kuid_t uid = sock ?
+               SOCK_INODE(sock)->i_uid :
+               make_kuid(sock_net(sk)->user_ns, 0);
+
+       sock_init_data_uid(sock, sk, uid);
+}
 EXPORT_SYMBOL(sock_init_data);

 void lock_sock_nested(struct sock *sk, int subclass)