Message-ID: <20250814161718.GA3117411-robh@kernel.org>
Date: Thu, 14 Aug 2025 11:17:18 -0500
From: Rob Herring <robh@...nel.org>
To: Daniel Stone <daniel@...ishbar.org>
Cc: Tomeu Vizoso <tomeu@...euvizoso.net>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Oded Gabbay <ogabbay@...nel.org>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Christian König <christian.koenig@....com>,
	Robin Murphy <robin.murphy@....com>,
	Steven Price <steven.price@....com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v2 2/2] accel: Add Arm Ethos-U NPU driver

On Thu, Aug 14, 2025 at 11:51:44AM +0100, Daniel Stone wrote:
> Hi Rob,

Thanks for the review.

> 
> On Tue, 12 Aug 2025 at 13:53, Daniel Stone <daniel@...ishbar.org> wrote:
> > On Mon, 11 Aug 2025 at 22:05, Rob Herring (Arm) <robh@...nel.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.

Yep. And there's a 3rd issue that I'm not setting 'ret' to an error 
value.

> 
> > > +       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.

What would be the usecase for exporting BOs here?

I suppose if one wants to feed in camera data and we need to do the 
allocation in the ethos driver since it likely has more constraints 
(i.e. must be contiguous). (Whatever happened on the universal allocator 
or constraint solver? I haven't been paying attention for a while...)

> 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.

I had considered clearing unused the region registers. That really has 
little effect. There's not any way to disable regions. And region 
offsets are a full 64-bits, so even if one set base address to 0 or some 
faulting region, a cmdstream can still get to any address.

The other issue is just whether there's leftover cmdstream state from 
prior jobs. That's why the cmd_info is initialized to all 1s so that the 
cmdstream has to setup all the state.

> > (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.

Here's the reworked (but not yet tested) code which I think should solve 
all of the above issues. There was also an issue with the cleanup path 
that we wouldn't do a put on the last BO if there was a size error. We 
just need to set ejob->region_bo[ejob->region_cnt] and increment 
region_cnt before any checks.

	ejob->cmd_bo = drm_gem_object_lookup(file, job->cmd_bo);
	if (!ejob->cmd_bo) {
		ret = -ENOENT;
		goto out_cleanup_job;
	}
	cmd_info = to_ethos_bo(ejob->cmd_bo)->info;
	if (!cmd_info) {
		ret = -EINVAL;
		goto out_cleanup_job;
	}

	for (int i = 0; i < NPU_BASEP_REGION_MAX; i++) {
		struct drm_gem_object *gem;

		/* Can only omit a BO handle if the region is not used or used for SRAM */
		if (!job->region_bo_handles[i] &&
		    (!cmd_info->region_size[i] || (i == ETHOS_SRAM_REGION && job->sram_size)))
			continue;

		gem = drm_gem_object_lookup(file, job->region_bo_handles[i]);
		if (!gem) {
			dev_err(dev->dev,
				"Invalid BO handle %d for region %d\n",
				job->region_bo_handles[i], i);
			ret = -ENOENT;
			goto out_cleanup_job;
		}

		ejob->region_bo[ejob->region_cnt] = gem;
		ejob->region_bo_num[ejob->region_cnt] = i;
		ejob->region_cnt++;

		if (to_ethos_bo(gem)->info) {
			dev_err(dev->dev,
				"Cmdstream BO handle %d used for region %d\n",
				job->region_bo_handles[i], i);
			ret = -EINVAL;
			goto out_cleanup_job;
		}

		/* Verify the command stream doesn't have accesses outside the BO */
		if (cmd_info->region_size[i] > gem->size) {
			dev_err(dev->dev,
				"cmd stream region %d size greater than BO size (%llu > %zu)\n",
				i, cmd_info->region_size[i], gem->size);
			ret = -EOVERFLOW;
			goto out_cleanup_job;
		}
	}

Powered by blists - more mailing lists