Re: [PATCH] mm: actually remap enough memory

[Grazvydas Ignotas <notasas@gmail.com>]

On Mon, Feb 9, 2015 at 11:23 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sun,  8 Feb 2015 04:55:12 +0200 Grazvydas Ignotas <notasas@gmail.com> wrote:
>
>> For whatever reason, generic_access_phys() only remaps one page, but
>> actually allows to access arbitrary size. It's quite easy to trigger
>> large reads, like printing out large structure with gdb, which leads to
>> a crash. Fix it by remapping correct size.
>>
>> ...
>>
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3829,7 +3829,7 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
>>       if (follow_phys(vma, addr, write, &prot, &phys_addr))
>>               return -EINVAL;
>>
>> -     maddr = ioremap_prot(phys_addr, PAGE_SIZE, prot);
>> +     maddr = ioremap_prot(phys_addr, PAGE_ALIGN(len + offset), prot);
>>       if (write)
>>               memcpy_toio(maddr + offset, buf, len);
>>       else
>
> hm, shouldn't this be PAGE_ALIGN(len)?

follow_phys() only returns page aligned address directly from page
table, so offset has to be added either to phys_addr or len. For
example if you need to read 4 bytes at address 0x10ffe or similar, 2
pages need to be mapped.

> Do we need the PAGE_ALIGN at all?  It's probably safer/saner to have it
> there, but x86 (at least) should be OK with arbitrary alignment on both
> addr and len?

Yes it's not strictly needed, but I'd prefer to keep it, as there is
already an assumption that ioremap operates in page quantities by
giving it page aligned phys_addr from follow_phys(). Or we could use
phys_addr + offset and len as arguments instead, no strong opinion
here.


Gražvydas

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>