linux-kernel - Re: [PATCH v2] signal: Adjust error codes according to restore_user

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: <CABeXuvouBzZuNarmNcd9JgZgvonL1N_p21gat=O_x0-1hMx=6A@mail.gmail.com>
Date:   Tue, 28 May 2019 13:47:28 -0700
From:   Deepa Dinamani <deepa.kernel@...il.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     David Laight <David.Laight@...lab.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        "dbueso@...e.de" <dbueso@...e.de>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        Davidlohr Bueso <dave@...olabs.net>, Eric Wong <e@...24.org>,
        Jason Baron <jbaron@...mai.com>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        linux-aio <linux-aio@...ck.org>,
        Omar Kilani <omar.kilani@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v2] signal: Adjust error codes according to restore_user_sigmask()

On Mon, May 27, 2019 at 8:04 AM Oleg Nesterov <oleg@...hat.com> wrote:
>
> Deepa,
>
> it seems that we both are saying the same things again and again, and we
> simply can't understand each other.

Oleg, I'm sorry for the confusion.  Maybe I should point out what I
agree with also.

I agree that signal handller being called and return value not being
altered is an issue with other syscalls also. I was just wondering if
some userspace code assumption would be assuming this. This is not a
kernel bug.

But, I do not think we have an understanding of what was wrong in
854a6ed56839a anymore since you pointed out that my assumption was not
correct that the signal handler being called without errno being set
is wrong.

One open question: this part of epoll_pwait was already broken before
854a6ed56839a. Do you agree?

if (err == -EINTR) {
                   memcpy(&current->saved_sigmask, &sigsaved,
                          sizeof(sigsaved));
                    set_restore_sigmask();
  } else
                   set_current_blocked(&sigsaved);

What to do next?
We could just see if your optimization patch resolves Eric's issue.
Or, I could revert the signal_pending() check and provide a fix
something like below(not a complete patch) since mainline has this
regression. Eric had tested something like this works also. And, I can
continue to look at what was wrong with 854a6ed56839a in the first
place. Let me know what you prefer:

-void restore_user_sigmask(const void __user *usigmask, sigset_t *sigsaved)
+int restore_user_sigmask(const void __user *usigmask, sigset_t
*sigsaved, int sig_pending)
 {

        if (!usigmask)
               return;

        /*
         * When signals are pending, do not restore them here.
         * Restoring sigmask here can lead to delivering signals that the above
         * syscalls are intended to block because of the sigmask passed in.
         */
+       if (sig_pending) {
                current->saved_sigmask = *sigsaved;
                set_restore_sigmask();
               return;
           }

@@ -2330,7 +2330,8 @@ 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,
error == -EINTR);

-Deepa