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

["Christian König" <christian.koenig@amd.com>]

On 15.08.25 13:15, Daniel Stone wrote:
> Hi Rob,
> 
> On Thu, 14 Aug 2025 at 17:17, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Aug 14, 2025 at 11:51:44AM +0100, Daniel Stone wrote:
>>> 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...)
> 
> Yeah, I guess it's just reasonably natural to allow export if you're
> allowing import as well.

I only partially agree, allowing export only makes sense if you have device memory which is not already covered by DMA-buf heaps.

So if you have special on chip memory which is accessible by for example a PCIe BAR then go ahead and provide an exporter for that.

But if your HW only needs CMA system memory then it is most likely better to use DMA-buf heaps instead and only provide an import functionality.

You can of course directly use the CMA code from your driver as well if you still want to support standard DRM, V4L or accel interfaces.

Regards,
Christian.

> 
>> 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.
> 
> Nice, thanks! That all looks good to me.
> 
> Using unchecked add/mul when calculating the sizes also made me raise
> an eyebrow - it might be provably safe but perhaps it's better to use
> all the helpers just to make sure undetected overflow can't occur.
> 
> Cheers,
> Daniel