linux-kernel - Re: [PATCH 3/4] seccomp: Support atomic "addfd + send reply"

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: <20210517181035.GH1964106@cisco>
Date:   Mon, 17 May 2021 12:10:35 -0600
From:   Tycho Andersen <tycho@...ho.pizza>
To:     Sargun Dhillon <sargun@...gun.me>
Cc:     Kees Cook <keescook@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux.dev>,
        Andy Lutomirski <luto@...nel.org>,
        Rodrigo Campos <rodrigo@...volk.io>,
        Mauricio Vásquez Bernal 
        <mauricio@...volk.io>, Giuseppe Scrivano <gscrivan@...hat.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Mickaël Salaün <mic@...ux.microsoft.com>
Subject: Re: [PATCH 3/4] seccomp: Support atomic "addfd + send reply"

On Mon, May 17, 2021 at 10:53:55AM -0700, Sargun Dhillon wrote:
> On Tue, May 11, 2021 at 2:50 PM Tycho Andersen <tycho@...ho.pizza> wrote:
> > > The struct seccomp_notif_resp, used when doing SECCOMP_IOCTL_NOTIF_SEND
> > > ioctl() to send a response to the target, has three more fields that we
> > > don't allow to set when doing the addfd ioctl() to also return. The
> > > reasons to disallow each field are:
> > >  * val: This will be set to the new allocated fd. No point taking it
> > >    from userspace in this case.
> > >  * error: If this is non-zero, the value is ignored. Therefore,
> > >    it is pointless in this case as we want to return the value.
> > >  * flags: The only flag is to let userspace continue to execute the
> > >    syscall. This seems pointless, as we want the syscall to return the
> > >    allocated fd.
> > >
> > > This is why those fields are not possible to set when using this new
> > > flag.
> >
> > I don't quite understand this; you don't need a NOTIF_SEND at all
> > with the way this currently works, right?
> >
> I reworded:
> 
> This effectively combines SECCOMP_IOCTL_NOTIF_ADDFD and
> SECCOMP_IOCTL_NOTIF_SEND into an atomic opteration. The notification's
> return value, nor error can be set by the user. Upon successful invocation
> of the SECCOMP_IOCTL_NOTIF_ADDFD ioctl with the SECCOMP_ADDFD_FLAG_SEND
> flag, the notifying process's errno will be 0, and the return value will
> be the file descriptor number that was installed.
> 
> How does that sound?

Works for me, thanks!

> > > @@ -1113,7 +1136,7 @@ static int seccomp_do_user_notification(int this_syscall,
> > >                                                struct seccomp_kaddfd, list);
> > >               /* Check if we were woken up by a addfd message */
> > >               if (addfd)
> > > -                     seccomp_handle_addfd(addfd);
> > > +                     seccomp_handle_addfd(addfd, &n);
> > >
> > >       }  while (n.state != SECCOMP_NOTIFY_REPLIED);
> > >
> >
> > This while() bit is introduced in the previous patch, can we fold this
> > deletion into that somehow?
> I'm not sure what you're getting at. This just an argument change which
> also passes the notification to the addfd function. The patch is split out
> to allow it to be backported to stable.

Yeah, I was mis-reading, you can ignore this. Sorry for the noise.

If you send another version, you can call the series:

Acked-by: Tycho Andersen <tycho@...ho.pizza>

Tycho