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;
 

-- 
2.25.1

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)

On Fri, 3 Feb 2023 at 15:58, Eric Dumazet <edumazet@google.com> wrote:
>
> 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()
>

I agree this is a much cleaner solution.
I'll wait the usual 24h for further comments, and send a patch like this in v3.

Best regards,
Pietro