Date: Wed, 6 Dec 2017 15:29:42 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jhibbits@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326611 - head/sys/sys Message-ID: <20171206133235.U2427@besplex.bde.org> In-Reply-To: <201712060205.vB625Lps036354@repo.freebsd.org> References: <201712060205.vB625Lps036354@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 6 Dec 2017, Justin Hibbits wrote:
> Log:
> Use unsigned intptr_t type for framebuffer addresses
>
> Summary:
> Some architectures (powerpc Book-E) have a vm_paddr_t larger than intptr_t.
> Casting from the intptr_t to vm_paddr_t causes sign extension, leading to a
> potentially invalid address.
Don't do that then. It is only valid to cast intptr_t to and from void *
or vm_offset_t. All of these can only represent virtual addresses, and
converting virtual addresses to physical addresses by casting to vm_paddr_t
gives garbage.
The problem is that fb_pbase is a physical address misrepresented using
[u]intptr_t. intptr_t for the virtual address fb_vbase is only a problem
if fb_base is misrepresented by a larger unsigned type.
> This was seen when running X on a PowerPC P1022 machine, which mapped the
> backing framebuffer at 0xc1800000. When mmap()d by X, this yielded an invalid
> address of 0xffffffffc1800000, or, as the hardware would see it, 0xfc1800000.
On 32-bit arches, truncation still occurs at 4G (insted of sign extension at
2G).
The sign extension bug would fix exotic arches where device memory is
below 2G or above top_of_memory-2G. amd64 doesn't seem to be one of these.
It puts the kernel near the top and uses sign extension tricks to reach it,
but that is only virtual.
> ...
> Modified: head/sys/sys/fbio.h
> ==============================================================================
> --- head/sys/sys/fbio.h Wed Dec 6 02:00:09 2017 (r326610)
> +++ head/sys/sys/fbio.h Wed Dec 6 02:05:21 2017 (r326611)
> @@ -136,8 +136,8 @@ struct fb_info {
> fb_leave_t *leave;
> fb_setblankmode_t *setblankmode;
>
> - intptr_t fb_pbase; /* For FB mmap. */
> - intptr_t fb_vbase; /* if NULL, use fb_write/fb_read. */
> + uintptr_t fb_pbase; /* For FB mmap. */
> + uintptr_t fb_vbase; /* if NULL, use fb_write/fb_read. */
This breaks the warning for fb_pbase. fb_pbase is a physical address, so
it cannot be represented by any intptr_t in general. Apparently it never
holds a virtual address, since that would be confusing even if it is
converted between virtual and physical correctly.
fb_vbase is apparently a virtual address, so intptr_t was good enough for
it.
The broken code in dev/fb using these struct members is:
X creator_vt.c: sc->fb.fb_pbase = phys;
'phys' has type bus_addr_t, so this assignment blindly truncates addresses
on arches where bus_address_t != [u]intptr_t. The sign extension even fixes
this on arches where intptr_t == int32_t, bus_addr_t == uint64_t, and
physical addresses are in the lower or upper 2G of the bus space.
Even vm_paddr_t is at least technically wrong for fb_pbase. Since the buffer
is a frame buffer, its bus space is probably always memory, and bus_addr_t
must be at least as large as vm_addr_t to represent any physical memory
space, but bus_addr_t might be larger and actually use the extra bits to
select a mapping method... Assignment is only correct if fb_base has type
bus_addr_t and is treated as a cookie, but for mmap() of device memory there
would have to be translation.
X fbd.c: if (info->fb_pbase == 0)
X fbd.c: *paddr = vtophys((uint8_t *)info->fb_vbase + offset);
X fbd.c: else
X fbd.c: *paddr = info->fb_pbase + offset;
This is the translation code in fb_mmap(). 'offset' is the mmap offset
which has type vm_ooffset_t which is hopefully large enough to represent
any physical address, although of course it isn't (it is too small for
/dev/kmem of 64-bit arches and also for /dev/mem on 64-bit arches if
high physical addresses are used).
The fb_pbase == 0 case partly correct. fb_vbase gives a virtual address
and 'offset' must be small to give another virtual address, and the result
is converted to a physical address. No overflow checking is done for
the addition.
fb_pbase != 0 means that fb_pbase gives a raw bus space address. 'offset'
is presumably still small, but fb_pbase is already broken since it can
only represent virtual addresses. As usual, no overflow checking is done
for the addition. The assignment is also logically broken. The RHS should
have type bus_addr_t, but mmap()'s API doesn't support that. *paddr has
type vm_paddr_t, which might have a different size and representation.
Callers have lots of casts to intptr_t for initializations of fb_vbase.
These were not quite correct. They are mostly/always to convert from
pointers to integral types to intptr_t, but intptr_t is only guaranteed
to work for converting between void * and intptr_t. The sloppy casts
works on vaxes because all pointers have the same representation. Now
these casts have the additional error of a sign mismatch between the
cast and the target type. This can cause sign extension bugs like the
one fixed (I think intptr_t and uintptr_t can have different sizes.
E.g., uintptr_t might be uint32_t but intptr_t might be int33_t or
more likely int64_t to avoid sign extension bugs, or reverse this to
expose sign extension bugs).
Assignments to fb_pbase are usually not cast. The first one found by
grep is perfect except for the broken target type. arm/.../*lcd.c
assigns sc->sc_fb_phys to fb_pbase. The RHS has the correct type
bus_addr_t, but the LHS can only represent virtual addresses.
The second one is logically broken. arm/.../*fimd.c assigns fb_pbase
to 'int reg;'. This works iff physical addresses are representable
by ints on arm (perhaps using sign extension). Similarly in all MD
code. dev/vt/.../ofwfb.c hard-codes a virtual == physical mapping
on powerpc. "Early" output in sc and fb uses even harder coding.
dev/drm2/.../radeon_fb.c assumes that 'resource_size_t aper_base'
plus an offsets fits in fb_pbase.
(A base isn't a size, but drm2 doesn't have a suitable type for bases,
so it abuses its resource_size_t which is vm_paddr_t on FreeBSD. I
think it should use rman_res_t which is uintmax_t. It was you that
expaned rman_res_t to uintmax_t to make it work with resource spaces
larger than the virtual address space. Anything smaller than
uintmax_t risks not working if any of the other types involved is
larger than the others. bus_addr_t has the same problem. But these
types could be MD and as small as possible (uint32_t on i386 without
PAE).)
drm2 also has bugs printing these types. For fb_pbase, it assumes that
the type is uintptr_t and uses the PRIXPTR mistake. This is now just
a style bug since the type is now unsigned. For aper_base, it uses a
bogus cast to unsigned long to break the warning and the value when the
value is large.
dev/terasic/.../*mtl_vt.c starts with an rman_res_t and truncates to
[u]intptr_t. pmap_mapdev() takes a vm_paddr_t, but is called with the
truncated copy. The correct conversions are approximately
rman_res_t -> vm_paddr_t (no change in value) -> bus_addr_t (maybe change
value) in fb_pbase; then back to vm_paddr_t to call pmap_mapdev() and
put the result in fb_vbase.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171206133235.U2427>