Message-ID: <CAG48ez1YjoQMe-daQ8NSqN46STGw1UWygzU2-qo75FLBDBqaow@mail.gmail.com>
Date: Tue, 5 Nov 2024 15:46:37 +0100
From: Jann Horn <jannh@...gle.com>
To: Liviu Dudau <liviu.dudau@....com>
Cc: Boris Brezillon <boris.brezillon@...labora.com>, Steven Price <steven.price@....com>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH] drm/panthor: Be stricter about IO mapping flags

On Tue, Nov 5, 2024 at 10:56 AM Liviu Dudau <liviu.dudau@....com> 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@...r.kernel.org
> > Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> > Signed-off-by: Jann Horn <jannh@...gle.com>
> > ---
> > 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;
}
```

Powered by blists - more mailing lists