linux-kernel - Re: [PATCH] gup: return -EFAULT on access

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: <CA+55aFwEqnY_Z5T-5UUwbxNJfV5MmfV=-8r73xvBnA1tnU_d_w@mail.gmail.com>
Date:   Thu, 5 Apr 2018 11:43:27 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>,
        syzbot+6304bf97ef436580fede@...kaller.appspotmail.com,
        linux-mm <linux-mm@...ck.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Huang Ying <ying.huang@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Thorsten Leemhuis <regressions@...mhuis.info>
Subject: Re: [PATCH] gup: return -EFAULT on access_ok failure

On Thu, Apr 5, 2018 at 11:28 AM, Michael S. Tsirkin <mst@...hat.com> wrote:
>
> to repeat what you are saying IIUC __get_user_pages_fast returns 0 if it can't
> pin any pages and that is by design.  Returning 0 on error isn't usual I think
> so I guess this behaviour should we well documented.

Arguably it happens elsewhere too, and not just in the kernel.
"read()" at past the end of a file is not an error, you'll just get 0
for EOF.

So it's not really "returning 0 on error".

It really is simply returning the number of pages it got. End of
story. That number of pages can be smaller than the requested number
of pages, and _that_ is due to some error, but note how it can return
"5" on error too - you asked for 10 pages, but the error happened in
the middle!

So the right way to check for error is to bverify that you get the
number of pages that you asked for. If you don't, something bad
happened.

Of course, many users don't actually care about "I didn't get
everything". They only care about "did I get _something_". Then that 0
ends up being the error case, but note how it depends on the caller.

> What about get_user_pages_fast though?

We do seem to special-case the first page there. I'm not sure it's a
good idea. But like the __get_user_pages_fast(), we seem to have users
that know about the particular semantics and depend on it.

It's all ugly, I agree.

End result: we can't just change semantics of either of them.

At least not without going through every single user and checking that
they are ok with it.

Which I guess I could be ok with. Maybe changing the semantics of
__get_user_pages_fast() is acceptable, if you just change it
*everywhere* (which includes not just he users, but also the couple of
architecture-specific versions of that same function that we have.

                    Linus