[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180208040455.GC14918@bombadil.infradead.org>
Date: Wed, 7 Feb 2018 20:04:55 -0800
From: Matthew Wilcox <willy@...radead.org>
To: Jann Horn <jannh@...gle.com>
Cc: linux-mm@...ck.org,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
kernel list <linux-kernel@...r.kernel.org>,
"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [RFC] Warn the user when they could overflow mapcount
On Thu, Feb 08, 2018 at 03:56:26AM +0100, Jann Horn wrote:
> How much memory would you need to trigger this? You need one
> vm_area_struct per increment, and those are 200 bytes? So at least
> 800GiB of memory for the vm_area_structs, and maybe more for other
> data structures?
That's a good point that I hadn't considered. Systems with that quantity
of memory are becoming available though.
> On systems with RAM on the order of terabytes, it's probably a good
> idea to turn on refcount hardening to make issues like that
> non-exploitable for now.
_mapcount is a bad candidate to be turned into a refcount_t. It's
completely legitimate to go to 0 and then back to 1. Also, we care
about being able to efficiently notice when it goes from 2 to 1 and
then from 1 to 0 (and we currently do that by biasing the count by -1).
I suppose it wouldn't be too hard to notice when we go from 0x7fff'ffff
to 0x8000'0000 and saturate the counter there.
> > That seems pretty bad. So here's a patch which adds documentation to the
> > two sysctls that a sysadmin could use to shoot themselves in the foot,
> > and adds a warning if they change either of them to a dangerous value.
>
> I have negative feelings about this patch, mostly because AFAICS:
>
> - It documents an issue instead of fixing it.
I prefer to think of it as warning the sysadmin they're doing something
dangerous, rather than preventing them from doing it ...
> - It likely only addresses a small part of the actual problem.
By this, you mean that there's a more general class of problem, and I make
no attempt to address it?
> > + if ((INT_MAX / max_map_count) > pid_max)
> > + pr_warn("pid_max is dangerously large\n");
>
> This in reordered is "if (pid_max * max_map_count < INT_MAX)
> pr_warn(...);", no? That doesn't make sense to me. Same thing again
> further down.
I should get more sleep before writing patches.
> > - if (unlikely(mm->map_count >= sysctl_max_map_count)) {
> > + if (unlikely(mm->map_count >= max_map_count)) {
>
> Why the renaming?
Because you can't have a function and an integer with the same name,
and the usual pattern we follow is that sysctl_foo_bar() is the function
to handle the variable foo_bar.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
