lkml.org 
[lkml]   [2022]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] vmap(): don't allow invalid pages
From
On 2022-01-19 16:27, Matthew Wilcox wrote:
> On Wed, Jan 19, 2022 at 01:28:14PM +0000, Robin Murphy wrote:
>>> + if (WARN_ON(!pfn_valid(page_to_pfn(page))))
>>
>> Is it page_to_pfn() guaranteed to work without blowing up if page is invalid
>> in the first place? Looking at the CONFIG_SPARSEMEM case I'm not sure that's
>> true...
>
> Even if it does blow up, at least it's blowing up here where someone
> can start to debug it, rather than blowing up on first access, where
> we no longer have the invlid struct page pointer.

But if that were the case then we'd blow up on the following line when
mk_pte(page, prot) ends up calling it same anyway. Indeed it's arguably
the best-case scenario since it would also blow up in page_address() if
we hit the vmap_range loop rather than going down the
vmap_small_pages_range_noflush() path.

Furthermore, if you *are* lucky enough to take a fault upon accessing a
bogus mapping, then surely a phys_to_page() calculation on whatever
ended up in the PTE should get you back the original "pointer" anyway,
shouldn't it? Sure it's a bit more work to extract the caller out of the
VMA if necessary, but hey, that's debugging! Maybe vmap() failing means
you then pass the offending nonsense to __free_pages() and that ruins
your day anyway...

The implications are infinitely worse if you've mapped something that
did happen to be a valid page, but wasn't the *right* page, such that
you don't fault but corrupt things or trigger a fatal system error
instead. I'd echo Mark's point that if we can't trust a page pointer to
be correct then we've already lost. In general the proportion of "wrong"
pointers one can viably attempt to detect is so small that it's rarely
ever worth trying, and the cases that are so obviously wrong tend to
show up well enough in normal operation (although NULL-safety is of
course a bit of a special case when it can simplify API usage).

> I don't think we have a 'page_valid' function which will tell us whether
> a random pointer is actually a struct page or not.

Indeed, my impression is that the only legitimate way to get hold of a
page pointer without assumed provenance is via pfn_to_page(), which is
where pfn_valid() comes in. Thus pfn_valid(page_to_pfn()) really
*should* be a tautology.

I guess in this instance it would be technically feasible to implement a
function which checks "is this a correctly-aligned pointer within memmap
bounds", but IMO that would be a massive step in the wrong direction.
We're developers; sometimes we introduce bugs when developing code, and
it takes effort to debug them, but that still doesn't make it a good
idea to optimise normal code paths for the expectation of writing new
catastrophically-bad bugs. Plus logically if such a "page_valid()" check
could be justified at all then that should rightfully lead to a
churn-fest of adding it to pretty much every interface which accepts
page pointers - one half of vmap() is hardly special.

FWIW, If anything I reckon a DEBUG_VM option that made checks within
page_to_x/x_to_page as appropriate would help Yury's issue just as much,
while having the potential to be considerably more useful in general.

Cheers,
Robin.

\
 
 \ /
  Last update: 2022-01-19 19:45    [W:0.178 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site