netdev - Re: [PATCH bpf-next v5 4/9] bpf: Implement cgroup sockaddr hooks for unix sockets

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 26 Sep 2023 11:54:36 +0200
From: Daan De Meyer <daan.j.demeyer@...il.com>
To: Jordan Rife <jrife@...gle.com>
Cc: bpf@...r.kernel.org, kernel-team@...a.com, martin.lau@...ux.dev, 
	netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v5 4/9] bpf: Implement cgroup sockaddr hooks for
 unix sockets

> > @@ -1919,6 +1936,13 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> >               goto out;
> >
> >       if (msg->msg_namelen) {
> > +             err = BPF_CGROUP_RUN_PROG_UNIX_SENDMSG_LOCK(sk,
> > +                                                         msg->msg_name,
> > +                                                         &msg->msg_namelen,
> > +                                                         NULL);
> > +             if (err)
> > +                     goto out;
> > +
> >               err = unix_validate_addr(sunaddr, msg->msg_namelen);
> >               if (err)
> >                       goto out;
>
>
> Just an FYI, I /think/ this is going to introduce a bug similar to the one I'm
> addressing in my patch here:
>
> - https://lore.kernel.org/netdev/20230921234642.1111903-2-jrife@google.com/
>
> With this change, callers to sock_sendmsg() in kernel space would see their
> value of msg->msg_namelen change if they are using Unix sockets. While it's
> unclear if there are any real systems that would be impacted, it can't hurt to
> insulate callers from these kind of side-effects. I can update my my patch to
> account for possible changes to msg_namelen.

That would be great! I think it makes sense to apply the same concept to unix
sockets so insulating changes to the msg_namelen seems like the way to go.

> Also, with this patch series is it possible for AF_INET BPF hooks (connect4,
> sendmsg4, connect6, etc.) to modify the address length?

This is not yet allowed. We only allow changing the unix sockaddr length at the
moment. Maybe in the future we'd want to allow changing INET6 addr lengths
as well but currently we don't allow this.


On Tue, 26 Sept 2023 at 00:13, Jordan Rife <jrife@...gle.com> wrote:
>
> > @@ -1919,6 +1936,13 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
> >               goto out;
> >
> >       if (msg->msg_namelen) {
> > +             err = BPF_CGROUP_RUN_PROG_UNIX_SENDMSG_LOCK(sk,
> > +                                                         msg->msg_name,
> > +                                                         &msg->msg_namelen,
> > +                                                         NULL);
> > +             if (err)
> > +                     goto out;
> > +
> >               err = unix_validate_addr(sunaddr, msg->msg_namelen);
> >               if (err)
> >                       goto out;
>
>
> Just an FYI, I /think/ this is going to introduce a bug similar to the one I'm
> addressing in my patch here:
>
> - https://lore.kernel.org/netdev/20230921234642.1111903-2-jrife@google.com/
>
> With this change, callers to sock_sendmsg() in kernel space would see their
> value of msg->msg_namelen change if they are using Unix sockets. While it's
> unclear if there are any real systems that would be impacted, it can't hurt to
> insulate callers from these kind of side-effects. I can update my my patch to
> account for possible changes to msg_namelen.
>
> Also, with this patch series is it possible for AF_INET BPF hooks (connect4,
> sendmsg4, connect6, etc.) to modify the address length?