[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89i+QMipmfOywjAX2jqZGs80zorE6yKFOsi9rXQbToZLbhQ@mail.gmail.com>
Date: Fri, 3 Feb 2023 15:58:23 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Pietro Borrello <borrello@...g.uniroma1.it>
Cc: "David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Lorenzo Colitti <lorenzo@...gle.com>,
Stephen Hemminger <stephen@...workplumber.org>,
Cristiano Giuffrida <c.giuffrida@...nl>,
"Bos, H.J." <h.j.bos@...nl>, Jakob Koschel <jkl820.git@...il.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/2] tun: tun_chr_open(): correctly initialize
socket uid
On Fri, Feb 3, 2023 at 3:30 PM Pietro Borrello
<borrello@...g.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@...g.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)
Powered by blists - more mailing lists