Re: [PATCH 1/1] x86: add phys addr validity check for /dev/mem mmap

[Dave Hansen]

Thanks for dredging this back up!

On 08/14/2014 07:18 AM, Frantisek Hrbata wrote:
> +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> +{
> +	return addr + count <= __pa(high_memory);
> +}

Is this correct on 32-bit?  It would limit /dev/mem to memory below 896MB.

> +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> +{

Nit: please add units to things like "count".  len_bytes would be nice
for this kind of thing, especially since it's passed *with* a pfn it
would be easy to think it is a count in pages.

> +	/* pgoff + count overflow is checked in do_mmap_pgoff */
> +	pfn += count >> PAGE_SHIFT;
> +
> +	if (pfn >> BITS_PER_LONG - PAGE_SHIFT)
> +		return -EOVERFLOW;

Is this -EOVERFLOW correct?  It is called like this:

> static int mmap_mem(struct file *file, struct vm_area_struct *vma)
> {
>         if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
>                 return -EINVAL;

So I think we need to return true/false:0/1.  -EOVERFLOW would be true,
and that if() would pass.

> +	return phys_addr_valid(pfn << PAGE_SHIFT);
> +}

Maybe I'm dumb, but it took me a minute to figure out what you were
trying to do with the: "(pfn >> BITS_PER_LONG - PAGE_SHIFT)".  In any
case, I think it is wrong on 32-bit.

On 32-bit, BITS_PER_LONG=32, and PAGE_SIZE=12, and a paddr=0x100000000
or pfn=0x100000 (4GB) is perfectly valid with PAE enabled.  But, this
code pfn>>(32-12) would result in 0x1 and return -EOVERFLOW.

I think something like this would be easier to read and actually work on
32-bit:

static inline int arch_pfn_possible(unsigned long pfn)
{
 	unsigned long max_arch_pfn = 1UL << (boot_cpu_data.x86_phys_bits -
PAGE_SHIFT);
	return pfn < max_arch_pfn;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/