linux-kernel - Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP

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: <20240621-colossal-acoustic-cassowary-ee8d05@houat>
Date: Fri, 21 Jun 2024 11:00:53 +0200
From: "mripard@...nel.org" <mripard@...nel.org>
To: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>, 
	Singo Chang (張興國) <Singo.Chang@...iatek.com>, 
	"linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>, "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>, 
	"chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"jason-jh.lin@...iatek.corp-partner.google.com" <jason-jh.lin@...iatek.corp-partner.google.com>, 
	Jason-ch Chen (陳建豪) <Jason-ch.Chen@...iatek.com>, 
	Shawn Sung (宋孝謙) <Shawn.Sung@...iatek.com>, Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>, 
	"daniel@...ll.ch" <daniel@...ll.ch>, "jkardatzke@...gle.com" <jkardatzke@...gle.com>, 
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>, 
	Project_Global_Chrome_Upstream_Group <Project_Global_Chrome_Upstream_Group@...iatek.com>, "airlied@...il.com" <airlied@...il.com>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com" <matthias.bgg@...il.com>, 
	"angelogioacchino.delregno@...labora.com" <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP

Hi,

On Tue, Jun 11, 2024 at 09:13:03AM GMT, Jason-JH Lin (林睿祥) wrote:
> Hi Maxime,
> 
> [snip]
> 
> > > > > > ---
> > > > > > TODO:
> > > > > > 1) Drop MTK_DRM_IOCTL_GEM_CREATE and use DMA_HEAP_IOCTL_ALLOC
> > > > > > in
> > > > > > userspace
> > > > > > 2) DRM driver use secure mailbox channel to handle normal and
> > > > > > secure flow
> > > > > > 3) Implement setting mmsys routing table in the secure world
> > > > > > series
> > > > > 
> > > > > I'm not sure what you mean here. Why are you trying to upstream
> > > > > something that still needs to be removed from your patch
> > > > > series?
> > > > > 
> > > > 
> > > > Because their is too much patches need to be fixed in this
> > > > series,
> > > > so I
> > > > list down the remaining TODO items and send to review for the
> > > > other
> > > > patches.
> > > > 
> > > > Sorry for the bothering, I'll drop this at the next version.
> > > 
> > > If you don't intend to use it, we just shouldn't add it. Removing
> > > the
> > > TODO item doesn't make sense, even more so if heaps should be the
> > > way
> > > you handle this.
> > > 
> > 
> > Sorry for this misunderstanding.
> > 
> > I mean I'll remove the DRM_IOCTL_GEM_CREATE patch and then change
> > user
> > space calling DMA_HEAP_IOCTL_ALLOC to allocate buffer from secure
> > heap.
> > 
> 
> I have changed user space to use DMA_HEAP_IOCTL_ALLOC to allocate
> secure buffer, but I still encounter the problem of determining whether
> the buffer is secure in mediatek-drm driver to add some secure
> configure for hardware.
> 
> 
> As the comment in [1], dma driver won't provide API for use.
> [1]: 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240515112308.10171-3-yong.wu@mediatek.com/#25857255
> 
> 
> So I use name checking at [PATCH v6 3/7] like this currently:
> 
> struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device
> *dev,
>             struct dma_buf_attachment *attach, struct sg_table *sg)
> {
>     struct mtk_gem_obj *mtk_gem;
> 
>     /* check if the entries in the sg_table are contiguous */
>     if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
>         DRM_ERROR("sg_table is not contiguous");
>         return ERR_PTR(-EINVAL);
>     }
> 
>     mtk_gem = mtk_gem_init(dev, attach->dmabuf->size);
>     if (IS_ERR(mtk_gem))
>         return ERR_CAST(mtk_gem);
> 
> +   mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted",
> 10));
>     mtk_gem->dma_addr = sg_dma_address(sg->sgl);
> +   mtk_gem->size = attach->dmabuf->size;
>     mtk_gem->sg = sg;
> 
>     return &mtk_gem->base;
> }
> 
> But I want to change this name checking to the information brought from
> user space.
> I tried to use arg->flags to append the secure flag in user space and
> call drmPrimeHandleToFD() to pass it to DRM driver, but it will be
> blocked by at the beginning of the drm_prime_handle_to_fd_ioctl().

I agree with you, it's something to discuss mostly with the dma-buf
maintainers but it would be better to just set a flag on the dma-buf,
and use that flag whenever necessary.

It might be related to the recent work I did to introduce allocation
flags too:
https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)