[igt-dev] [i-g-t] igt/gem_mmap_offset: Adding subtest oob_read

[Matthew Auld]

On Wed, 9 Mar 2022 at 08:19, Mastan Katragadda
<mastanx.katragadda at intel.com> wrote:
>
> This test will Detect A missing bounds check in vm_access().
> can lead to an out-of-bounds read or write in the adjacent memory area.
>
> Signed-off-by: Mastan Katragadda <mastanx.katragadda at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> ---
>  tests/i915/gem_mmap_offset.c | 37 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c
> index 8148f0a2..9723f4e3 100644
> --- a/tests/i915/gem_mmap_offset.c
> +++ b/tests/i915/gem_mmap_offset.c
> @@ -289,6 +289,40 @@ static void *memchr_inv(const void *s, int c, size_t n)
>         return NULL;
>  }
>
> +static void
> +test_oob_read(int i915)
> +{
> +

Nit: Random newline.

> +       int memfd;
> +       unsigned char p_init[4096];
> +       unsigned char p_read_buf[4096];
> +       void *ptr;
> +       uintptr_t p_addr;
> +       int ret;
> +       uint32_t handle;

Nit: Try to keep these roughly ordered by line length and type, for
better readability.

unsigned char p_read_buf[4096];
unsigned char p_init[4096];
uintptr_t p_addr;
uint32_t handle;
void *ptr;
int memfd;
int ret;

> +
> +       handle = gem_create(i915, 4096);
> +
> +       memset(p_init, 0x41, sizeof(p_init));
> +       gem_write(i915, handle, 0, &p_init, sizeof(p_init));

Why do we need the memset() + gem_write()?

> +
> +       ptr = __gem_mmap_offset__wc(i915, handle, 0, 4096, PROT_READ|PROT_WRITE);

Can we make this __device_coherent, otherwise I think this skips, or
just fails on discrete? And since vm_access uses tm_bo_vm_access() on
such platforms, it would be good to get coverage there also.

> +       igt_assert(ptr != MAP_FAILED);

We can use the normal gem_mmap__device_coherent, which already checks
this for us?

> +
> +       memfd = open("/proc/self/mem", O_RDWR, 0);
> +       igt_require_f(memfd != -1, "/proc/self/mem\n");
> +
> +       p_addr = (uintptr_t)((unsigned long)ptr + 4092);
> +       ret = lseek64(memfd, p_addr, SEEK_SET);
> +       igt_assert_f(ret != -1, "lseek failed\n");
> +
> +       /* Triggering the buf (out of bound read) */
> +       ret = read(memfd, p_read_buf, sizeof(p_read_buf));
> +       igt_assert(ret == -1 && errno ==  EIO);
> +       munmap(ptr, 4096);
> +       close(memfd);

gem_close(i915, handle);

> +}
> +
>  static void test_ptrace(int i915)
>  {
>         const unsigned int SZ = 3 * 4096;
> @@ -692,6 +726,9 @@ igt_main
>         igt_subtest_f("pf-nonblock")
>                 pf_nonblock(i915);
>
> +       igt_subtest("oob-read")
> +               test_oob_read(i915);

Newly added tests should also ideally include an igt_describe(), to
give a brief overview of what the test is trying to exercise.

Looking at the CI results, it looks like this triggers the issue
nicely on the kernel side.

> +
>         igt_subtest_with_dynamic("ptrace")
>                 test_ptrace(i915);
>
> --
> 2.25.1
>