Re: [PATCH] accel/rocket: Report per-core engine usage through fdinfo

[Gyeyoung Baek]

Hi Sidong,

> If userspace (for example systemd-udevd reacting to the drm_dev_register()
> uevent, or a client opening the device while remaining cores are still probing
> or deferred) calls rocket_open() before all cores have finished probing,
> rocket_priv->engine_usage is allocated with the smaller rdev->num_cores count
> at that moment. Once the remaining cores finish probing and increment
> rdev->num_cores, calling rocket_show_fdinfo() will iterate up to the new
> rdev->num_cores and read past the end of elapsed_ns[].
>
> Similarly, if a lower-indexed core is unbound via rocket_remove(),
> rdev->num_cores is decremented while higher-indexed cores retain their
> original core->index. If a file is opened afterwards, rocket_job_handle_irq()
> indexing engine_usage->elapsed_ns[core->index] can also write out of bounds:
>

I reproduced the OOB access sashiko reported.
I used `driver_override` to keep core1 and core2 from
binding, so only core0 had been probed when the file was opened:

  BUG: KASAN: slab-out-of-bounds in rocket_show_fdinfo+0xac/0x120 [rocket]
  Read of size 8 at addr ffff00000242b230 by task grep/461

  Hardware name: Radxa ROCK 5B (DT)
  Call trace:
   rocket_show_fdinfo+0xac/0x120 [rocket]
   drm_show_fdinfo+0x218/0x2a8 [drm]
   seq_show+0x340/0x648
   seq_read_iter+0x318/0x9d8
   vfs_read+0x220/0x6a8
   ksys_read+0x108/0x1f8
   ...

  Allocated by task 444 on cpu 5 at 79.841431s:
   __kmalloc_noprof+0x2d0/0x740
   rocket_open+0xb4/0x468 [rocket]
   drm_file_alloc+0x37c/0x568 [drm]
   drm_open_helper+0x110/0x2a8 [drm]
   accel_open+0xc4/0x1c0 [drm]
   ...

> drivers/accel/rocket/rocket_job.c:rocket_job_handle_irq() {
>         ...
>         core->in_flight_job->engine_usage->elapsed_ns[core->index] +=
>                 local_clock() - core->in_flight_job->start_ns;
>         ...
> }
>
> Would it make sense to record the total core count allocated for rdev->cores
> in rocket_device_init() (or store the allocated array length in struct
> rocket_engine_usage) so that the allocation size and bounds stay consistent?
>

`num_cores` is used with two different meanings:

  1. in rocket_device_init(): the number of core nodes in the DT
         (the size of the cores[] allocation)
  2. rdev->num_cores: the number of cores currently bound

This patch uses (2) where (1) is needed. How about storing (1) in a
new member (e.g. max_cores), as sashiko suggested, and using it instead?

> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261031083642.1076356-1-sidong.yang@xxxxxxxxxx?part=1


--
Thanks,
Gyeyoung