On Tue, Nov 5, 2024 at 10:56 AM Liviu Dudau <liviu.dudau@xxxxxxx> wrote:
> On Tue, Nov 05, 2024 at 12:17:13AM +0100, Jann Horn wrote:
> > The current panthor_device_mmap_io() implementation has two issues:
> >
> > 1. For mapping DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET,
> > panthor_device_mmap_io() bails if VM_WRITE is set, but does not clear
> > VM_MAYWRITE. That means userspace can use mprotect() to make the mapping
> > writable later on. This is a classic Linux driver gotcha.
> > I don't think this actually has any impact in practice:
> > When the GPU is powered, writes to the FLUSH_ID seem to be ignored; and
> > when the GPU is not powered, the dummy_latest_flush page provided by the
> > driver is deliberately designed to not do any flushes, so the only thing
> > writing to the dummy_latest_flush could achieve would be to make *more*
> > flushes happen.
> >
> > 2. panthor_device_mmap_io() does not block MAP_PRIVATE mappings (which are
> > mappings without the VM_SHARED flag).
> > MAP_PRIVATE in combination with VM_MAYWRITE indicates that the VMA has
> > copy-on-write semantics, which for VM_PFNMAP are semi-supported but
> > fairly cursed.
> > In particular, in such a mapping, the driver can only install PTEs
> > during mmap() by calling remap_pfn_range() (because remap_pfn_range()
> > wants to **store the physical address of the mapped physical memory into
> > the vm_pgoff of the VMA**); installing PTEs later on with a fault
> > handler (as panthor does) is not supported in private mappings, and so
> > if you try to fault in such a mapping, vmf_insert_pfn_prot() splats when
> > it hits a BUG() check.
> >
> > Fix it by clearing the VM_MAYWRITE flag (userspace writing to the FLUSH_ID
> > doesn't make sense) and requiring VM_SHARED (copy-on-write semantics for
> > the FLUSH_ID don't make sense).
> >
> > Reproducers for both scenarios are in the notes of my patch on the mailing
> > list; I tested that these bugs exist on a Rock 5B machine.
> >
> > Note that I only compile-tested the patch, I haven't tested it; I don't
> > have a working kernel build setup for the test machine yet. Please test it
> > before applying it.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> > ---
> > First testcase (can write to the FLUSH_ID):
> >
> > ```
> >
>
> There is a missing line here, I guess is something like
>
> #define SYSCHK(x) ({ \
Oops. Yes, sorry, the tool that I stored this comment message in
interpreted all lines starting with "#" as comments... the proper
versions:
First testcase (can write to the FLUSH_ID):
```
#include <err.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdint.h>
#include <sys/mman.h>
#define SYSCHK(x) ({ \
typeof(x) __res = (x); \
if (__res == (typeof(x))-1) \
err(1, "SYSCHK(" #x ")"); \
__res; \
})
#define GPU_PATH "/dev/dri/by-path/platform-fb000000.gpu-card"
#define DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET (1ull << 56)
int main(void) {
int fd = SYSCHK(open(GPU_PATH, O_RDWR));
// sanity-check that PROT_WRITE+MAP_SHARED fails
void *mmap_write_res = mmap(NULL, 0x1000, PROT_READ|PROT_WRITE,
MAP_SHARED, fd, DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET);
if (mmap_write_res == MAP_FAILED) {
perror("mmap() with PROT_WRITE+MAP_SHARED failed as expected");
} else {
errx(1, "mmap() with PROT_WRITE+MAP_SHARED worked???");
}
// make a PROT_READ+MAP_SHARED mapping, and upgrade it to writable
void *mmio_page = SYSCHK(mmap(NULL, 0x1000, PROT_READ, MAP_SHARED,
fd, DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET));
SYSCHK(mprotect(mmio_page, 0x1000, PROT_READ|PROT_WRITE));
volatile uint32_t *flush_counter = (volatile uint32_t*)mmio_page;
uint32_t last_old = -1;
while (1) {
uint32_t old_val = *flush_counter;
*flush_counter = 1111;
uint32_t new_val = *flush_counter;
if (old_val != last_old)
printf("flush counter: old=%u, new=%u\n", old_val, new_val);
last_old = old_val;
}
}
```
Second testcase (triggers BUG() splat):
```
#include <err.h>
#include <fcntl.h>
#include <stddef.h>
#include <sys/mman.h>
#define SYSCHK(x) ({ \
typeof(x) __res = (x); \
if (__res == (typeof(x))-1) \
err(1, "SYSCHK(" #x ")"); \
__res; \
})
#define GPU_PATH "/dev/dri/by-path/platform-fb000000.gpu-card"
#define DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET (1ull << 56)
int main(void) {
int fd = SYSCHK(open(GPU_PATH, O_RDWR));
// make a PROT_READ+**MAP_PRIVATE** mapping
void *ptr = SYSCHK(mmap(NULL, 0x1000, PROT_READ, MAP_PRIVATE,
fd, DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET));
// trigger a read fault
*(volatile char *)ptr;
}
```
