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 <[email protected]>
---
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
<[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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