linux-kernel - Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse

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: <CAAeHK+xK2Nb6J6YbvUJdXaZoecB0GS2UyY6pgGwrfCoOQJ34xg@mail.gmail.com>
Date:   Tue, 2 Oct 2018 15:19:57 +0200
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Will Deacon <will.deacon@....com>,
        Kostya Serebryany <kcc@...gle.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Chintan Pandya <cpandya@...eaurora.org>,
        Shuah Khan <shuah@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Jacob Bramley <Jacob.Bramley@....com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Evgenii Stepanov <eugenis@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Ruben Ayrapetyan <Ruben.Ayrapetyan@....com>,
        Ramana Radhakrishnan <Ramana.Radhakrishnan@....com>,
        Robin Murphy <robin.murphy@....com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        linux-mm <linux-mm@...ck.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lee Smith <Lee.Smith@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse

On Fri, Sep 28, 2018 at 7:50 PM, Catalin Marinas
<catalin.marinas@....com> wrote:
> On Mon, Sep 17, 2018 at 07:01:00PM +0200, Andrey Konovalov wrote:

>> Looking at patch #8 ("usb, arm64: untag user addresses in devio") in
>> this series, it seems that that devio ioctl actually accepts a pointer
>> into a vma, so we shouldn't actually be untagging its argument and the
>> patch needs to be dropped.
>
> You are right, the pointer seems to have originated from the kernel as
> already untagged (mmap() on the driver), so we would expect the user to
> pass it back an untagged pointer.

OK, dropped this patch in v7.

>> As for case 1, the places where pointers are compared with TASK_SIZE
>> and others can be found with grep. Maybe it makes sense to introduce
>> some kind of routine like is_user_pointer() that handles tagged
>> pointers and refactor the existing code to use it? And maybe add a
>> rule to checkpatch.pl that forbids the direct usage of TASK_SIZE and
>> others.
>>
>> So I think detecting direct comparisons with TASK_SIZE and others
>> would more useful than finding __user pointer casts (it seems that the
>> latter requires a lot of annotations to be fixed/added), and I should
>> just drop this patch with annotations.
>
> I think point (1) is not too bad, usually found with grep.
>
> As I've said in my previous reply, I kind of came to the same conclusion
> that searching __user pointer casts to long may not actually scale. If
> we could add an __untagged annotation to ulong where it matters (e.g.
> find_vma()), we could identify a ulong (default tagged) and annotate
> some of those.
>
> However, this analysis on __user * casting was useful even if we don't
> end up using it. If we come up with a clearer definition of the ABI
> (which syscalls accept tagged pointers), we may conclude that the only
> places where untagging matters are a few find_vma() calls in the arch
> and mm code and can ignore the rest.

So what exactly should I do now?

For now I've posted v7 with the sparse annotation patch dropped (to
have the most up-do-date version posted).