linux-kernel - Re: [PATCH v2 2/2] accel: Add Arm Ethos-U NPU driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPj87rNDPQqTqj1LAdFYmd4Y12UHXWi5+65i0RepkcOX3wvEyA@mail.gmail.com>
Date: Thu, 14 Aug 2025 11:51:44 +0100
From: Daniel Stone <daniel@...ishbar.org>
To: "Rob Herring (Arm)" <robh@...nel.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

Hi Rob,

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.

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