netdev - Re: [PATCH v1 net] af_unix: Call sk_diag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20221125014905.98714-1-kuniyu@amazon.com>
Date:   Fri, 25 Nov 2022 10:49:04 +0900
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <harperchen1110@...il.com>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <kuni1840@...il.com>, <kuniyu@...zon.com>,
        <netdev@...r.kernel.org>, <pabeni@...hat.com>,
        <syzkaller@...glegroups.com>
Subject: Re: [PATCH v1 net] af_unix: Call sk_diag_fill() under the bucket lock.

From:   Wei Chen <harperchen1110@...il.com>
Date:   Thu, 24 Nov 2022 17:37:04 +0800
> Dear Linux developers,
> 
> My step tracing over Linux found the following C program would trigger
> the reported crash. I hope it is helpful for bug fix.

Thank you, Wei.

I guess you commented out the sock_diag_check_cookie() validation ?
Otherwise you would have to set req.udr.udiag_cookie.

For the record, my repro:

---8<---
#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/un.h>
#include <linux/netlink.h>
#include <linux/rtnetlink.h>
#include <linux/sock_diag.h>
#include <linux/unix_diag.h>

void main(void)
{
	struct sockaddr_nl nladdr = {
		.nl_family = AF_NETLINK
	};
	struct {
		struct nlmsghdr nlh;
		struct unix_diag_req udr;
	} req = {
		.nlh = {
			.nlmsg_len = sizeof(req),
			.nlmsg_type = SOCK_DIAG_BY_FAMILY,
			.nlmsg_flags = NLM_F_REQUEST
		},
		.udr = {
			.sdiag_family = AF_UNIX,
			.udiag_show = UDIAG_SHOW_UID,
		}
	};
	struct iovec iov = {
		.iov_base = &req,
		.iov_len = sizeof(req)
	};
	struct msghdr msg = {
		.msg_name = &nladdr,
		.msg_namelen = sizeof(nladdr),
		.msg_iov = &iov,
		.msg_iovlen = 1
	};
	int netlink_fd, unix_fd, ret;
	struct stat file_stat;
	socklen_t optlen;
	__u64 cookie;

	unix_fd = socket(AF_UNIX, SOCK_STREAM, 0);
	fstat(unix_fd, &file_stat);
	optlen = sizeof(cookie);
	getsockopt(unix_fd, SOL_SOCKET, SO_COOKIE, &cookie, &optlen);

	req.udr.udiag_ino = file_stat.st_ino;
	req.udr.udiag_cookie[0] = (__u32)cookie;
	req.udr.udiag_cookie[1] = (__u32)(cookie >> 32);

	netlink_fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
	sendmsg(netlink_fd, &msg, 0);

	close(netlink_fd);
	close(unix_fd);
}
---8<---


Interestingly, I only see the NULL deref with this patch applied.
Anyway, I'll post a fix later :)

---8<---
diff --git a/net/unix/diag.c b/net/unix/diag.c
index 105f522a89fe..f1c8f565af77 100644
--- a/net/unix/diag.c
+++ b/net/unix/diag.c
@@ -117,6 +117,7 @@ static int sk_diag_show_rqlen(struct sock *sk, struct sk_buff *nlskb)
 static int sk_diag_dump_uid(struct sock *sk, struct sk_buff *nlskb)
 {
 	uid_t uid = from_kuid_munged(sk_user_ns(nlskb->sk), sock_i_uid(sk));
+	printk(KERN_ERR "sk_diag_dump_uid: sk: %px\tskb->sk: %px, %px\n", sk, nlskb->sk, sk_user_ns(nlskb->sk));
 	return nla_put(nlskb, UNIX_DIAG_UID, sizeof(uid_t), &uid);
 }
 
---8<---


> 
> #include <errno.h>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <linux/netlink.h>
> #include <linux/rtnetlink.h>
> #include <linux/sock_diag.h>
> #include <linux/unix_diag.h>
> #include <linux/stat.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> 
> int main(void) {
>     int fd1 = socket(AF_UNIX, SOCK_STREAM, 0);
>     struct stat file_stat;
>     fstat(fd1, &file_stat);
>     int fd2 = socket(AF_NETLINK, SOCK_RAW, NETLINK_SOCK_DIAG);
> 
>     struct sockaddr_nl nladdr = {
>         .nl_family = AF_NETLINK
>     };
>     struct {
>         struct nlmsghdr nlh;
>         struct unix_diag_req udr;
>     } req = {
>         .nlh = {
>             .nlmsg_len = sizeof(req),
>             .nlmsg_type = SOCK_DIAG_BY_FAMILY,
>             .nlmsg_flags = NLM_F_REQUEST
>         },
>         .udr = {
>             .sdiag_family = AF_UNIX,
>             .udiag_states = -1,
>             .udiag_ino = file_stat.st_ino,
>             .udiag_show = 0x40
>         }
>     };
>     struct iovec iov = {
>         .iov_base = &req,
>         .iov_len = sizeof(req)
>     };
>     struct msghdr msg = {
>         .msg_name = &nladdr,
>         .msg_namelen = sizeof(nladdr),
>         .msg_iov = &iov,
>         .msg_iovlen = 1
>     };
> 
>     sendmsg(fd2, &msg, 0);
>     return 0;
> }
> 
> Best,
> Wei
> 
> On Wed, 23 Nov 2022 at 23:38, Paolo Abeni <pabeni@...hat.com> wrote:
> >
> > On Wed, 2022-11-23 at 07:22 -0800, Kuniyuki Iwashima wrote:
> > > From:   Wei Chen <harperchen1110@...il.com>
> > > Date:   Wed, 23 Nov 2022 23:09:53 +0800
> > > > Dear Paolo,
> > > >
> > > > Could you explain the meaning of modified "ss" version to reproduce
> > > > the bug? I'd like to learn how to reproduce the bug in the user space
> > > > to facilitate the bug fix.
> > >
> > > I think it means to drop NLM_F_DUMP and modify args as needed because
> > > ss dumps all sockets, not exactly a single socket.
> >
> > Exactly! Additionally 'ss' must fill udiag_ino and udiag_cookie with
> > values matching a live unix socket. And before that you have to add
> > more code to allow 'ss' dumping such values (or fetch them with some
> > bpf/perf probe).
> >
> > >
> > > Ah, I misunderstood that the found sk is passed to sk_user_ns(), but it's
> > > skb->sk.
> >
> > I did not double check the race you outlined in this patch. That could
> > still possibly be a valid/existing one.
> >
> > > P.S.  I'm leaving for Japan today and will be bit slow this and next week
> > > for vacation.
> >
> > Have a nice trip ;)
> >
> > /P