linux-kernel - Re: [PATCH v3 1/2] seccomp: Add wait_killable semantic to seccomp user notifier

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: <CACaBj2ba08sQTvJSTuApiXAcsRsC09UMbA3AN744r6FEngBGow@mail.gmail.com>
Date:   Tue, 3 May 2022 16:27:18 +0200
From:   Rodrigo Campos <rodrigo@...volk.io>
To:     Sargun Dhillon <sargun@...gun.me>
Cc:     Kees Cook <keescook@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux Containers <containers@...ts.linux-foundation.org>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Will Drewry <wad@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        Alban Crequy <alban@...volk.io>
Subject: Re: [PATCH v3 1/2] seccomp: Add wait_killable semantic to seccomp
 user notifier

On Mon, May 2, 2022 at 6:04 PM Sargun Dhillon <sargun@...gun.me> wrote:
>
> On Mon, May 02, 2022 at 04:15:07PM +0200, Rodrigo Campos wrote:
> > (I couldn't git-am this locally, so maybe I'm injecting this at the
> > wrong parts mentally when looking at the other code for more context.
> > Sorry if that is the case :))
> >
> > Why do we need to complete() only in this error path? As far as I can
> > see this is on the error path where the copy to userspace failed and
> > we want to reset this notification.
> This error path acts as follows
> (Say, S: Supervisor, P: Notifying Process, U: User)
>
> P: 2 <-- Pid
> P: getppid() // Generated notification
> P: Waiting in wait_interruptible state
> S: Calls receive notification, and the codepath gets up to the poin
>    where it's copying the notification to userspace
> U: kill -SIGURG 2 // Send a kill signal to the notifying process
> P: Waiting in the wait_killable state
> S: Kernel fails to copy notification into user memory, and resets
>    the notification and returns an error
>
> If we do not have the reset, the P will never return to wait interruptible.

Ohhh, because we want the wait to be interruptible again! Right, I
forgot we should reset to that state again, until the notification is
indeed handled.

What if we say something along those lines in the comment, then? Like:

// Make the other side go back to wait interruptible, the notification
is not SENT.

Something like that would at least help me in the future :)

> > We _need_ to call complete() in the non error path here so the other
> > side wakes up and switches to a killable wait. As we are not doing
> > this (for the non error path), this will basically not achieve a
> > wait_killable() at all.
> >
> No, because here, we check if we were waiting interruptible, and
> then we switch to wait_killable:

Ohhh, right right right. This is lazily changing to wait killable only
when something already wakes up the process. Sorry, I overlooked that.



Best,
Rodrigo