linux-kernel - Re: Strange issues with epoll since 5.0

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]

Message-ID: <CABeXuvqbCDhp+67SpGLAO7dYiWzWufewQBn+MTxY5NYsaQVrPg@mail.gmail.com>
Date:   Wed, 1 May 2019 13:53:57 -0700
From:   Deepa Dinamani <deepa.kernel@...il.com>
To:     Eric Wong <e@...24.org>
Cc:     Davidlohr Bueso <dave@...olabs.net>, Arnd Bergmann <arnd@...db.de>,
        Al Viro <viro@...iv.linux.org.uk>,
        Jason Baron <jbaron@...mai.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Omar Kilani <omar.kilani@...il.com>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>
Subject: Re: Strange issues with epoll since 5.0

On Wed, May 1, 2019 at 1:48 PM Eric Wong <e@...24.org> wrote:
>
> Deepa Dinamani <deepa.kernel@...il.com> wrote:
> > So here is my analysis:
>
> <snip everything I agree with>
>
> > So the 854a6ed56839a40f6 seems to be better than the original code in
> > that it detects the signal.
>
> OTOH, does matter to anybody that a signal is detected slightly
> sooner than it would've been, otherwise?

The original code drops the signal altogether. This is because it
overwrites the current's sigmask with the provided
one(set_current_blocked()). If a signal bit was set, it is lost
forever. It does not detect it sooner. The check for pending signal is
sooner and not just before the syscall returns.
This is what the patch in discussion does: check for signals just
before returning.

>
> > But, the problem is that it doesn't
> > communicate it to the userspace.
>
> Yup, that's a big problem :)
>
> > So a patch like below solves the problem. This is incomplete. I'll
> > verify and send you a proper fix you can test soon. This is just for
> > the sake of discussion:
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index 4a0e98d87fcc..63a387329c3d 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -2317,7 +2317,7 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct
> > epoll_event __user *, events,
> >                 int, maxevents, int, timeout, const sigset_t __user *, sigmask,
> >                 size_t, sigsetsize)
> >  {
> > -       int error;
> > +       int error, signal_detected;
> >         sigset_t ksigmask, sigsaved;
> >
> >         /*
> > @@ -2330,7 +2330,10 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct
> > epoll_event __user *, events,
> >
> >         error = do_epoll_wait(epfd, events, maxevents, timeout);
> >
> > -       restore_user_sigmask(sigmask, &sigsaved);
> > +       signal_detected = restore_user_sigmask(sigmask, &sigsaved);
> > +
> > +       if (signal_detected && !error)
> > +               return -EITNR;
> >
> >         return error;
>
> Looks like a reasonable API.
>
> > @@ -2862,7 +2862,7 @@ void restore_user_sigmask(const void __user
> > *usigmask, sigset_t *sigsaved)
> >         if (signal_pending(current)) {
> >                 current->saved_sigmask = *sigsaved;
> >                 set_restore_sigmask();
> > -               return;
> > +               return 0;
>
> Shouldn't that "return 1" if a signal is pending?

Yep, I meant this to be 1.

-Deepa