Hi Rob,
On Tue, 12 Aug 2025 at 13:53, Daniel Stone daniel@fooishbar.org wrote:
On Mon, 11 Aug 2025 at 22:05, Rob Herring (Arm) robh@kernel.org wrote:
+static int ethos_ioctl_submit_job(struct drm_device *dev, struct drm_file *file,
struct drm_ethos_job *job)+{
[...]ejob->cmd_bo = drm_gem_object_lookup(file, job->cmd_bo);cmd_info = to_ethos_bo(ejob->cmd_bo)->info;if (!ejob->cmd_bo)goto out_cleanup_job;NULL deref here if this points to a non-command BO. Which is better than wild DMA, but hey.
Sorry this wasn't more clear. There are two NULL derefs here. If you pass an invalid BO, ejob->cmd_bo is dereferenced before the NULL check, effectively neutering it and winning you a mail from the other Dan when he runs sparse on it. Secondly you pass a BO which is valid but not a command BO, cmd_info gets unconditionally dereferenced so it will fall apart there too.
for (int i = 0; i < NPU_BASEP_REGION_MAX; i++) {struct drm_gem_object *gem;if (job->region_bo_handles[i] == 0)continue;/* Don't allow a region to point to the cmd BO */if (job->region_bo_handles[i] == job->cmd_bo) {ret = -EINVAL;goto out_cleanup_job;}And here I suppose you want to check if the BO's info pointer is non-NULL, i.e. disallow use of _any_ command BO instead of only disallowing this job's own command BO.
This is the main security issue, since it would allow writes a cmdstream BO which has been created but is not _the_ cmdstream BO for this job. Fixing that is pretty straightforward, but given that someone will almost certainly try to add dmabuf support to this driver, it's also probably worth a comment in the driver flags telling anyone who tries to add DRIVER_PRIME that they need to disallow export of cmdbuf BOs.
Relatedly, I think there's missing validity checks around the regions. AFAICT it would be possible to do wild memory access: * create a cmdstream BO which accesses one region * submit a job using that cmdstream with one data BO correctly attached to the region, execute the job and wait for completion * free the data BO * resubmit that job but declare zero BO handles
The first issue is that the job will be accepted by the processing ioctl, because it doesn't check that all the regions specified by the cmdstream are properly filled in by the job, which is definitely one to fix for validation. The second issue is that region registers are not cleared in any way, so in the above example, the second job will reuse the region configuration from the first. I'm not sure if clearing out unused job fields would be helpful defence in depth or not; your call.
(There's also a NULL deref if an invalid GEM handle is specified.)
This one is similar to the first; drm_gem_object_lookup() return isn't checked so it gets dereferenced unconditionally.
Cheers, Daniel