linux-kernel - Re: [RFC 3/8] mm: Avoid using set_page_count() in set_page

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]

Date:   Wed, 27 Oct 2021 14:27:01 -0400
From:   Pasha Tatashin <pasha.tatashin@...een.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, linux-mm <linux-mm@...ck.org>,
        linux-m68k@...ts.linux-m68k.org,
        Anshuman Khandual <anshuman.khandual@....com>,
        Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        william.kucharski@...cle.com,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        schmitzmic@...il.com, Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Roman Gushchin <guro@...com>,
        Muchun Song <songmuchun@...edance.com>, weixugc@...gle.com,
        Greg Thelen <gthelen@...gle.com>
Subject: Re: [RFC 3/8] mm: Avoid using set_page_count() in set_page_recounted()

On Wed, Oct 27, 2021 at 1:12 AM John Hubbard <jhubbard@...dia.com> wrote:
>
> On 10/26/21 11:21, Pasha Tatashin wrote:
> > It must return the same thing, if it does not we have a bug in our
> > kernel which may lead to memory corruptions and security holes.
> >
> > So today we have this:
> >     VM_BUG_ON_PAGE(page_ref_count(page), page); -> check ref_count is 0
> >     < What if something modified here? Hmm..>
> >     set_page_count(page, 1); -> Yet we reset it to 1.
> >
> > With my proposed change:
> >     VM_BUG_ON_PAGE(page_ref_count(page), page); -> check ref_count is 0
> >     refcnt = page_ref_inc_return(page);  -> ref_count better be 1.
> >     VM_BUG_ON_PAGE(refcnt != 1, page); -> Verify that it is 1.
> >
>
> Yes, you are just repeating what the diffs say.
>
> But it's still not good to have this function name doing something completely
> different than its name indicates.

I see, I can rename it to: 'set_page_recounted/get_page_recounted' ?

>
> >>
> >> I understand where this patchset is going, but this intermediate step is
> >> not a good move.
> >>
> >> Also, for the overall series, if you want to change from
> >> "set_page_count()" to "inc_and_verify_val_equals_one()", then the way to
> >> do that is *not* to depend solely on VM_BUG*() to verify. Instead,
> >> return something like -EBUSY if incrementing the value results in a
> >> surprise, and let the caller decide how to handle it.
> >
> > Actually, -EBUSY would be OK if the problems were because we failed to
> > modify refcount for some reason, but if we modified refcount and got
> > an unexpected value (i.e underflow/overflow) we better report it right
> > away instead of waiting for memory corruption to happen.
> >
>
> Having the caller do the BUG() or VM_BUG*() is not a significant delay.

We cannot guarantee that new callers in the future will check return
values, the idea behind this work is to ensure that we are always
protected from refcount underflow/overflow and invalid refcount
modifications by set_refcount.

Pasha