| Message ID | 20240515112308.10171-1-yong.wu@mediatek.com |
|---|---|
| Headers | show |
| Series | dma-buf: heaps: Add restricted heap | expand |
Am 15.05.24 um 13:23 schrieb Yong Wu: > Introduce a FLAG for the restricted memory which means the memory is > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > Currently we don't use sg_dma_unmark_restricted, thus this interface > has not been added. Why should that be part of the scatterlist? It doesn't seem to affect any of it's functionality. As far as I can see the scatterlist shouldn't be the transport of this kind of information. Regards, Christian. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > include/linux/scatterlist.h | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 77df3d7b18a6..a6ad9018eca0 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct scatterlist *sg) > > #define SG_DMA_BUS_ADDRESS (1 << 0) > #define SG_DMA_SWIOTLB (1 << 1) > +#define SG_DMA_RESTRICTED (2 << 1) > > /** > * sg_dma_is_bus_address - Return whether a given segment was marked > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > sg->dma_flags |= SG_DMA_SWIOTLB; > } > > +/** > + * sg_dma_mark_restricted - Mark the scatterlist for restricted buffer. > + * @sg: SG entry > + * > + * Description: > + * Marks a a scatterlist for the restricted buffer that may be inaccessiable > + * in kernel if it is protected. > + */ > +static inline void sg_dma_mark_restricted(struct scatterlist *sg) > +{ > + sg->dma_flags |= SG_DMA_RESTRICTED; > +} > + > +/** > + * sg_dma_is_restricted - Return whether the scatterlist was marked as restricted > + * buffer. > + * @sg: SG entry > + * > + * Description: > + * Returns true if the scatterlist was marked as restricted buffer. > + */ > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return sg->dma_flags & SG_DMA_RESTRICTED; > +} > #else > > static inline bool sg_dma_is_bus_address(struct scatterlist *sg) > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > { > } > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return false; > +} > + > +static inline void sg_dma_mark_restrited(struct scatterlist *sg) > +{ > +} > #endif /* CONFIG_NEED_SG_DMA_FLAGS */ > > /**
Il 15/05/24 13:23, Yong Wu ha scritto: > Introduce a FLAG for the restricted memory which means the memory is > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > Currently we don't use sg_dma_unmark_restricted, thus this interface > has not been added. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > include/linux/scatterlist.h | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 77df3d7b18a6..a6ad9018eca0 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct scatterlist *sg) > > #define SG_DMA_BUS_ADDRESS (1 << 0) > #define SG_DMA_SWIOTLB (1 << 1) > +#define SG_DMA_RESTRICTED (2 << 1) I think you wanted to write (1 << 2) here :-) Cheers, Angelo > > /** > * sg_dma_is_bus_address - Return whether a given segment was marked > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > sg->dma_flags |= SG_DMA_SWIOTLB; > } > > +/** > + * sg_dma_mark_restricted - Mark the scatterlist for restricted buffer. > + * @sg: SG entry > + * > + * Description: > + * Marks a a scatterlist for the restricted buffer that may be inaccessiable > + * in kernel if it is protected. > + */ > +static inline void sg_dma_mark_restricted(struct scatterlist *sg) > +{ > + sg->dma_flags |= SG_DMA_RESTRICTED; > +} > + > +/** > + * sg_dma_is_restricted - Return whether the scatterlist was marked as restricted > + * buffer. > + * @sg: SG entry > + * > + * Description: > + * Returns true if the scatterlist was marked as restricted buffer. > + */ > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return sg->dma_flags & SG_DMA_RESTRICTED; > +} > #else > > static inline bool sg_dma_is_bus_address(struct scatterlist *sg) > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > { > } > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return false; > +} > + > +static inline void sg_dma_mark_restrited(struct scatterlist *sg) > +{ > +} > #endif /* CONFIG_NEED_SG_DMA_FLAGS */ > > /**
On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Am 15.05.24 um 13:23 schrieb Yong Wu: > > Introduce a FLAG for the restricted memory which means the memory > is > > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > > > Currently we don't use sg_dma_unmark_restricted, thus this > interface > > has not been added. > > Why should that be part of the scatterlist? It doesn't seem to > affect > any of it's functionality. > > As far as I can see the scatterlist shouldn't be the transport of > this > kind of information. Thanks for the review. I will remove this. In our user scenario, DRM will import these buffers and check if this is a restricted buffer. If yes, it will use secure GCE takes over. If this judgment is not suitable to be placed in scatterlist. I don't know if it is ok to limit this inside dma-buf. Adding such an interface: static bool dma_buf_is_restricted(struct dma_buf *dmabuf) { return !strncmp(dmabuf->exp_name, "restricted", 10); } Thanks. > > Regards, > Christian. > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > include/linux/scatterlist.h | 34 > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/include/linux/scatterlist.h > b/include/linux/scatterlist.h > > index 77df3d7b18a6..a6ad9018eca0 100644 > > --- a/include/linux/scatterlist.h > > +++ b/include/linux/scatterlist.h > > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct > scatterlist *sg) > > > > #define SG_DMA_BUS_ADDRESS(1 << 0) > > #define SG_DMA_SWIOTLB(1 << 1) > > +#define SG_DMA_RESTRICTED(2 << 1) > > > > /** > > * sg_dma_is_bus_address - Return whether a given segment was > marked > > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct > scatterlist *sg) > > sg->dma_flags |= SG_DMA_SWIOTLB; > > } > > > > +/** > > + * sg_dma_mark_restricted - Mark the scatterlist for restricted > buffer. > > + * @sg:SG entry > > + * > > + * Description: > > + * Marks a a scatterlist for the restricted buffer that may be > inaccessiable > > + * in kernel if it is protected. > > + */ > > +static inline void sg_dma_mark_restricted(struct scatterlist *sg) > > +{ > > +sg->dma_flags |= SG_DMA_RESTRICTED; > > +} > > + > > +/** > > + * sg_dma_is_restricted - Return whether the scatterlist was > marked as restricted > > + * buffer. > > + * @sg:SG entry > > + * > > + * Description: > > + * Returns true if the scatterlist was marked as restricted > buffer. > > + */ > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > > +{ > > +return sg->dma_flags & SG_DMA_RESTRICTED; > > +} > > #else > > > > static inline bool sg_dma_is_bus_address(struct scatterlist *sg) > > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct > scatterlist *sg) > > { > > } > > > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > > +{ > > +return false; > > +} > > + > > +static inline void sg_dma_mark_restrited(struct scatterlist *sg) > > +{ > > +} > > #endif/* CONFIG_NEED_SG_DMA_FLAGS */ > > > > /** > >
On Thu, 2024-05-16 at 11:59 +0200, AngeloGioacchino Del Regno wrote: > Il 15/05/24 13:23, Yong Wu ha scritto: > > Introduce a FLAG for the restricted memory which means the memory > > is > > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > interface > > has not been added. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > include/linux/scatterlist.h | 34 > > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/include/linux/scatterlist.h > > b/include/linux/scatterlist.h > > index 77df3d7b18a6..a6ad9018eca0 100644 > > --- a/include/linux/scatterlist.h > > +++ b/include/linux/scatterlist.h > > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct > > scatterlist *sg) > > > > #define SG_DMA_BUS_ADDRESS (1 << 0) > > #define SG_DMA_SWIOTLB (1 << 1) > > +#define SG_DMA_RESTRICTED (2 << 1) > > I think you wanted to write (1 << 2) here :-) Apparently, you are right:) Thanks. > > Cheers, > Angelo
Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> Am 15.05.24 um 13:23 schrieb Yong Wu: >>> Introduce a FLAG for the restricted memory which means the memory >> is >>> protected by TEE or hypervisor, then it's inaccessiable for kernel. >>> >>> Currently we don't use sg_dma_unmark_restricted, thus this >> interface >>> has not been added. >> Why should that be part of the scatterlist? It doesn't seem to >> affect >> any of it's functionality. >> >> As far as I can see the scatterlist shouldn't be the transport of >> this >> kind of information. > Thanks for the review. I will remove this. > > In our user scenario, DRM will import these buffers and check if this > is a restricted buffer. If yes, it will use secure GCE takes over. > > If this judgment is not suitable to be placed in scatterlist. I don't > know if it is ok to limit this inside dma-buf. Adding such an > interface: > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > { > return !strncmp(dmabuf->exp_name, "restricted", 10); > } No, usually stuff like that doesn't belong into DMA buf either. Question here really is who controls the security status of the memory backing the buffer? In other words who tells the exporter that it should allocate and fill a buffer with encrypted data? If that is userspace then that is part of the format information and it is also userspace who should tell the importer that it needs to work with encrypted data. The kernel is intentionally not involved in stuff like that. Regards, Christian. > > Thanks. > >> Regards, >> Christian. >> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com> >>> --- >>> include/linux/scatterlist.h | 34 >> ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/include/linux/scatterlist.h >> b/include/linux/scatterlist.h >>> index 77df3d7b18a6..a6ad9018eca0 100644 >>> --- a/include/linux/scatterlist.h >>> +++ b/include/linux/scatterlist.h >>> @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct >> scatterlist *sg) >>> >>> #define SG_DMA_BUS_ADDRESS(1 << 0) >>> #define SG_DMA_SWIOTLB(1 << 1) >>> +#define SG_DMA_RESTRICTED(2 << 1) >>> >>> /** >>> * sg_dma_is_bus_address - Return whether a given segment was >> marked >>> @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct >> scatterlist *sg) >>> sg->dma_flags |= SG_DMA_SWIOTLB; >>> } >>> >>> +/** >>> + * sg_dma_mark_restricted - Mark the scatterlist for restricted >> buffer. >>> + * @sg:SG entry >>> + * >>> + * Description: >>> + * Marks a a scatterlist for the restricted buffer that may be >> inaccessiable >>> + * in kernel if it is protected. >>> + */ >>> +static inline void sg_dma_mark_restricted(struct scatterlist *sg) >>> +{ >>> +sg->dma_flags |= SG_DMA_RESTRICTED; >>> +} >>> + >>> +/** >>> + * sg_dma_is_restricted - Return whether the scatterlist was >> marked as restricted >>> + * buffer. >>> + * @sg:SG entry >>> + * >>> + * Description: >>> + * Returns true if the scatterlist was marked as restricted >> buffer. >>> + */ >>> +static inline bool sg_dma_is_restricted(struct scatterlist *sg) >>> +{ >>> +return sg->dma_flags & SG_DMA_RESTRICTED; >>> +} >>> #else >>> >>> static inline bool sg_dma_is_bus_address(struct scatterlist *sg) >>> @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct >> scatterlist *sg) >>> { >>> } >>> >>> +static inline bool sg_dma_is_restricted(struct scatterlist *sg) >>> +{ >>> +return false; >>> +} >>> + >>> +static inline void sg_dma_mark_restrited(struct scatterlist *sg) >>> +{ >>> +} >>> #endif/* CONFIG_NEED_SG_DMA_FLAGS */ >>> >>> /** >>
Hi Christian, On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote: > Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > > > > > External email : Please do not click links or open attachments > > > until > > > you have verified the sender or the content. > > > Am 15.05.24 um 13:23 schrieb Yong Wu: > > > > Introduce a FLAG for the restricted memory which means the > > > > memory > > > > > > is > > > > protected by TEE or hypervisor, then it's inaccessiable for > > > > kernel. > > > > > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > > > > > interface > > > > has not been added. > > > > > > Why should that be part of the scatterlist? It doesn't seem to > > > affect > > > any of it's functionality. > > > > > > As far as I can see the scatterlist shouldn't be the transport of > > > this > > > kind of information. > > > > Thanks for the review. I will remove this. > > > > In our user scenario, DRM will import these buffers and check if > > this > > is a restricted buffer. If yes, it will use secure GCE takes over. > > > > If this judgment is not suitable to be placed in scatterlist. I > > don't > > know if it is ok to limit this inside dma-buf. Adding such an > > interface: > > > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > > { > > return !strncmp(dmabuf->exp_name, "restricted", 10); > > } > > No, usually stuff like that doesn't belong into DMA buf either. > > Question here really is who controls the security status of the > memory > backing the buffer? > > In other words who tells the exporter that it should allocate and > fill a > buffer with encrypted data? > > If that is userspace then that is part of the format information and > it > is also userspace who should tell the importer that it needs to work > with encrypted data. > > The kernel is intentionally not involved in stuff like that. > Here is the expected protected content buffer flow in DRM: 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by DMA_HEAP_IOCTL_ALLOC. 2) userspace imports that dma-buf into the device using prime for the drm_file. 3) userspace uses the already implemented driver import code for the special cases of protected content buffer. In the step 3), we need to verify the dma-buf is allocated from "restricted_mtk_cma", but there is no way to pass the secure flag or private data from userspace to the import interface in DRM driver. So I can only verify it like this now: 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; } I think I have the same problem as the ECC_FLAG mention in: https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ I think it would be better to have the user configurable private information in dma-buf, so all the drivers who have the same requirement can get their private information from dma-buf directly and no need to change or add the interface. What's your opinion in this point? Regards, Jason-JH.Lin > Regards, > Christian.
> > > > Here is the expected protected content buffer flow in DRM: > > 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" > > by > > DMA_HEAP_IOCTL_ALLOC. > > 2) userspace imports that dma-buf into the device using prime for > > the > > drm_file. > > 3) userspace uses the already implemented driver import code for > > the > > special cases of protected content buffer. > > What is so special on that case? The special case simply means the the protected content buffer. > > > In the step 3), we need to verify the dma-buf is allocated from > > "restricted_mtk_cma", but there is no way to pass the secure flag > > or > > private data from userspace to the import interface in DRM driver. > > Why do you need to verify that? I need to know the imported buffer is allocated from restricted cma and mark it as a secure buffer in mediatek-drm driver. Then, I will add some configuration to the hardware if the buffer is secure buffer, so that it can get the permission to access the secure buffer. > > > So I can only verify it like this now: > > 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; > > } > > Complete NAK from my side to that approach. Importing of a DMA-buf > should be independent of the exporter. > > What you could do is to provide the secure buffer from a device and > not a device heap. > You mean I should allocate buffer in mediate-drm driver not userspace? I just have modified this to userspace by the comment here: https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.369-3-shawn.sung@mediatek.com/#25806766 > > I think I have the same problem as the ECC_FLAG mention in: > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > I think it would be better to have the user configurable private > > information in dma-buf, so all the drivers who have the same > > requirement can get their private information from dma-buf directly > > and > > no need to change or add the interface. > > > > What's your opinion in this point? > > Well of hand I don't see the need for that. > > What happens if you get a non-secure buffer imported in your secure > device? We use the same mediatek-drm driver for secure and non-secure buffer. If non-secure buffer imported to mediatek-drm driver, it's go to the normal flow with normal hardware settings. We use different configurations to make hardware have different permission to access the buffer it should access. So if we can't get the information of "the buffer is allocated from restricted_mtk_cma" when importing the buffer into the driver, we won't be able to configure the hardware correctly. Regards, Jason-JH.Lin > > Regards, > Christian. > > > Regards, > > Jason-JH.Lin > > > > > Regards, > > > Christian. > > > > ************* MEDIATEK Confidentiality Notice > > ******************** > > The information contained in this e-mail message (including any > > attachments) may be confidential, proprietary, privileged, or > > otherwise > > exempt from disclosure under applicable laws. It is intended to be > > conveyed only to the designated recipient(s). Any use, > > dissemination, > > distribution, printing, retaining or copying of this e-mail > > (including its > > attachments) by unintended recipient(s) is strictly prohibited and > > may > > be unlawful. If you are not an intended recipient of this e-mail, > > or believe > > > > that you have received this e-mail in error, please notify the > > sender > > immediately (by replying to this e-mail), delete any and all copies > > of > > this e-mail (including any attachments) from your system, and do > > not > > disclose the content of this e-mail to any other person. Thank you! >
On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > I think it would be better to have the user configurable private > > > > information in dma-buf, so all the drivers who have the same > > > > requirement can get their private information from dma-buf directly > > > > and > > > > no need to change or add the interface. > > > > > > What's your opinion in this point? > > > > Well of hand I don't see the need for that. > > > > What happens if you get a non-secure buffer imported in your secure > > > device? > > > > We use the same mediatek-drm driver for secure and non-secure buffer. > > If non-secure buffer imported to mediatek-drm driver, it's go to the > > normal flow with normal hardware settings. > > > > We use different configurations to make hardware have different > > permission to access the buffer it should access. > > > > So if we can't get the information of "the buffer is allocated from > > restricted_mtk_cma" when importing the buffer into the driver, we won't > > be able to configure the hardware correctly. > > Why can't you get this information from userspace? Same reason amd and i915/xe also pass this around internally in the kernel, it's just that for those gpus the render and kms node are the same driver so this is easy. But on arm you have split designs everywhere and dma-buf import/export, so something else is needed. And neither current kms uapi nor protocols/extensions have provisions for this (afaik) because it works on the big gpus, and on android it's just hacked up with backchannels. So yeah essentially I think we probably need something like this, as much as it sucks. I see it somewhat similar to handling pcip2pdma limitations in the kernel too. Not sure where/how it should be handled though, and maybe I've missed something around protocols, in which case I guess we should add some secure buffer flags to the ADDFB2 ioctl. -Sima
On Wed, 2024-06-26 at 12:49 +0200, Christian König wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > > "restricted_mtk_cma", but there is no way to pass the > > secure flag > > > > or > > > > private data from userspace to the import interface in DRM > > driver. > > > > > > Why do you need to verify that? > > > > I need to know the imported buffer is allocated from restricted cma > > and > > mark it as a secure buffer in mediatek-drm driver. Then, I will add > > some configuration to the hardware if the buffer is secure buffer, > > so > > that it can get the permission to access the secure buffer. > > Yeah so far that makes sense. This is basically what other drivers do > with secure buffers as well. > > But why do you want the kernel to transport that information? Usually > drivers get the information from userspace what to do with a buffer. > > In other words the format, stride, tilling and also if it's a secure > buffer or not comes from userspace. > Thanks for your clear explanation. I think this is what I want, but I can't find any DRM interface to pass the secure flag from userspace. > What the hardware usually handles internally is things like > encryption keys, but you eventually get the information where to look > for the key from userspace as well. > > Handling inside the kernel would only be necessary if userspace could > for example crash the system with invalid parameters. But for > encryption that is usually not the case. > Yes, that's true. > > > > > > > So I can only verify it like this now: > > > > 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; > > > > } > > > > > > Complete NAK from my side to that approach. Importing of a DMA- > > buf > > > should be independent of the exporter. > > > > > > What you could do is to provide the secure buffer from a device > > and > > > not a device heap. > > > > > > > You mean I should allocate buffer in mediate-drm driver not > > userspace? > > Well that depends. The question is if you have multiple drivers which > needs to work with this secure buffer? > > If yes then you should have a general allocation heap for it. If no > then the buffers could as well be allocated from the driver interface > directly. > Yes, this buffer needs work with GPU and DRM drivers, so this general "restricted_mtk_cma" will allocated in userspace, then being passed to GPU and DRM. > > I just have modified this to userspace by the comment here: > > > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.369-3-shawn.sung@mediatek.com/#25806766 > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > I think it would be better to have the user configurable > > private > > > > information in dma-buf, so all the drivers who have the same > > > > requirement can get their private information from dma-buf > > directly > > > > and > > > > no need to change or add the interface. > > > > > > > > What's your opinion in this point? > > > > > > Well of hand I don't see the need for that. > > > > > > What happens if you get a non-secure buffer imported in your > > secure > > > device? > > > > We use the same mediatek-drm driver for secure and non-secure > > buffer. > > If non-secure buffer imported to mediatek-drm driver, it's go to > > the > > normal flow with normal hardware settings. > > > > We use different configurations to make hardware have different > > permission to access the buffer it should access. > > > > So if we can't get the information of "the buffer is allocated from > > restricted_mtk_cma" when importing the buffer into the driver, we > > won't > > be able to configure the hardware correctly. > > Why can't you get this information from userspace? As I mentioned here: https://patchwork.kernel.org/project/linux-mediatek/cover/20240525232928.5524-1-jason-jh.lin@mediatek.com/#25886488 I tried some DRM interfaces using buffer FD and arg->flag as parameters, but it didn't work. So I ask for your help here. But I think I should find DRM maintainer to add the secure flag to DRM interface now. Regards, Jason-JH.Lin
On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > I think it would be better to have the user configurable > private > > > > > information in dma-buf, so all the drivers who have the same > > > > > requirement can get their private information from dma-buf > directly > > > > > and > > > > > no need to change or add the interface. > > > > > > > What's your opinion in this point? > > > > > Well of hand I don't see the need for that. > > > > > What happens if you get a non-secure buffer imported in your > secure > > > > device? > > > > > > We use the same mediatek-drm driver for secure and non-secure > buffer. > > > If non-secure buffer imported to mediatek-drm driver, it's go to > the > > > normal flow with normal hardware settings. > > > > > > We use different configurations to make hardware have different > > > permission to access the buffer it should access. > > > > > > So if we can't get the information of "the buffer is allocated > from > > > restricted_mtk_cma" when importing the buffer into the driver, we > won't > > > be able to configure the hardware correctly. > > > > Why can't you get this information from userspace? > > Same reason amd and i915/xe also pass this around internally in the > kernel, it's just that for those gpus the render and kms node are the > same > driver so this is easy. > > But on arm you have split designs everywhere and dma-buf > import/export, so > something else is needed. And neither current kms uapi nor > protocols/extensions have provisions for this (afaik) because it > works on > the big gpus, and on android it's just hacked up with backchannels. > > So yeah essentially I think we probably need something like this, as > much > as it sucks. I see it somewhat similar to handling pcip2pdma > limitations > in the kernel too. > > Not sure where/how it should be handled though, and maybe I've missed > something around protocols, in which case I guess we should add some > secure buffer flags to the ADDFB2 ioctl. Thanks for your hint, I'll try to add the secure flag to the ADDFB2 ioctl. If it works, I'll send the patch. Regards, Jason-JH.Lin > -Sima
Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > I think it would be better to have the user configurable > > private > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > requirement can get their private information from dma-buf > > directly > > > > > > and > > > > > > no need to change or add the interface. > > > > > > > > What's your opinion in this point? > > > > > > Well of hand I don't see the need for that. > > > > > > What happens if you get a non-secure buffer imported in your > > secure > > > > > device? > > > > > > > > We use the same mediatek-drm driver for secure and non-secure > > buffer. > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > the > > > > normal flow with normal hardware settings. > > > > > > > > We use different configurations to make hardware have different > > > > permission to access the buffer it should access. > > > > > > > > So if we can't get the information of "the buffer is allocated > > from > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > won't > > > > be able to configure the hardware correctly. > > > > > > Why can't you get this information from userspace? > > > > Same reason amd and i915/xe also pass this around internally in the > > kernel, it's just that for those gpus the render and kms node are the > > same > > driver so this is easy. > > The reason I ask is that encryption here looks just like another parameter for the buffer, e.g. like format, stride, tilling etc.. So instead of this during buffer import: 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; You can trivially say during use hey this buffer is encrypted. At least that's my 10 mile high view, maybe I'm missing some extensive key exchange or something like that. > > > But on arm you have split designs everywhere and dma-buf > > import/export, so > > something else is needed. And neither current kms uapi nor > > protocols/extensions have provisions for this (afaik) because it > > works on > > the big gpus, and on android it's just hacked up with backchannels. > > > > So yeah essentially I think we probably need something like this, as > > much > > as it sucks. I see it somewhat similar to handling pcip2pdma > > limitations > > in the kernel too. > > > > Not sure where/how it should be handled though, and maybe I've missed > > something around protocols, in which case I guess we should add some > > secure buffer flags to the ADDFB2 ioctl. > > Thanks for your hint, I'll try to add the secure flag to the ADDFB2 > ioctl. If it works, I'll send the patch. Yeah, exactly what I would suggest as well. I'm not an expert for that part, but as far as I know we already have bunch of device specific tilling flags in there. Adding an MTK_ENCRYPTED flag should be trivial. Regards, Christian. > > Regards, > Jason-JH.Lin > > > -Sima > > ************* MEDIATEK Confidentiality Notice ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!
On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > External email : Please do not click links or open attachments > > until > > > you have verified the sender or the content. > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > requirement can get their private information from dma-buf > > > directly > > > > > > > and > > > > > > > no need to change or add the interface. > > > > > > > > > What's your opinion in this point? > > > > > > > Well of hand I don't see the need for that. > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > > > device? > > > > > > > > We use the same mediatek-drm driver for secure and > > non-secure > > > buffer. > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > > > normal flow with normal hardware settings. > > > > > > > > We use different configurations to make hardware have > > different > > > > > permission to access the buffer it should access. > > > > > > > > So if we can't get the information of "the buffer is > > allocated > > > from > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > > > be able to configure the hardware correctly. > > > > > > Why can't you get this information from userspace? > > > > Same reason amd and i915/xe also pass this around internally in the > > > kernel, it's just that for those gpus the render and kms node are the > > > same > > > driver so this is easy. > > > > > The reason I ask is that encryption here looks just like another parameter > for the buffer, e.g. like format, stride, tilling etc.. > > So instead of this during buffer import: > > 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; > > You can trivially say during use hey this buffer is encrypted. > > At least that's my 10 mile high view, maybe I'm missing some extensive key > exchange or something like that. That doesn't work in all cases, unfortunately. If you're doing secure video playback, the firmware is typically in charge of the frame decryption/decoding, and you'd get dma-buf back that aren't accessible by the CPU (or at least, not at the execution level Linux runs with). So nobody can map that buffer, and the firmware driver is the one who knows that this buffer cannot be accessed by anyone. Putting this on the userspace to know would be pretty weird, and wouldn't solve the case where the kernel would try to map it. Maxime
On Thu, Jun 27, 2024 at 04:40:02PM GMT, mripard@kernel.org wrote: > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > External email : Please do not click links or open attachments > > > until > > > > you have verified the sender or the content. > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > > I think it would be better to have the user configurable > > > > private > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > requirement can get their private information from dma-buf > > > > directly > > > > > > > > and > > > > > > > > no need to change or add the interface. > > > > > > > > > > What's your opinion in this point? > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > secure > > > > > > > device? > > > > > > > > > We use the same mediatek-drm driver for secure and > > > non-secure > > > > buffer. > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > the > > > > > > normal flow with normal hardware settings. > > > > > > > > > We use different configurations to make hardware have > > > different > > > > > > permission to access the buffer it should access. > > > > > > > > > So if we can't get the information of "the buffer is > > > allocated > > > > from > > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > > won't > > > > > > be able to configure the hardware correctly. > > > > > > > Why can't you get this information from userspace? > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > kernel, it's just that for those gpus the render and kms node are the > > > > same > > > > driver so this is easy. > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > for the buffer, e.g. like format, stride, tilling etc.. > > > > So instead of this during buffer import: > > > > 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; > > > > You can trivially say during use hey this buffer is encrypted. > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > exchange or something like that. > > That doesn't work in all cases, unfortunately. > > If you're doing secure video playback, the firmware is typically in > charge of the frame decryption/decoding, and you'd get dma-buf back that > aren't accessible by the CPU (or at least, not at the execution level > Linux runs with). Can you clarify which firmware you're talking about? Is this secure firmware, or firmware running on the video decoding hardware? > So nobody can map that buffer, and the firmware driver is the one who > knows that this buffer cannot be accessed by anyone. Putting this on the > userspace to know would be pretty weird, and wouldn't solve the case > where the kernel would try to map it. Doesn't userspace need to know from the start whether it's trying to do secure playback or not? Typically this involves more than just the decoding part. You'd typically set up things like HDCP as part of the process, so userspace probably already does know that the buffers being passed around are protected. Also, the kernel shouldn't really be mapping these buffers unless explicitly told to. In most cases you also wouldn't want the kernel to map these kinds of buffers, right? Are there any specific cases where you expect the kernel to need to map these? I've been looking at this on the Tegra side recently and the way it works on these chips is that you basically get an opaque carveout region that has been locked down by secure firmware or early bootloaders, so only certain hardware blocks can access it. We can allocate from that carveout and then pass the buffers around. It may be possible to use these protected carveout regions exclusively from the DRM/KMS driver and share them with multimedia engines via DMA- BUF, but I've also been looking into perhaps using DMA-BUF heaps to expose the carveout, which would make this a bit more flexible and allow either userspace to allocate the buffers or have multiple kernel drivers share the carveout via the DMA-BUF heap. Though the latter would require that there be in-kernel APIs for heaps, so not too sure about that yet. Thierry
On Wed, May 15, 2024 at 07:23:04PM GMT, Yong Wu wrote: > Add "struct restricted_heap_ops". For the restricted memory, totally there > are two steps: > a) alloc: Allocate the buffer in kernel; > b) restrict_buf: Restrict/Protect/Secure that buffer. > The "alloc" is mandatory while "restrict_buf" is optional since it may > be part of "alloc". > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/dma-buf/heaps/restricted_heap.c | 41 ++++++++++++++++++++++++- > drivers/dma-buf/heaps/restricted_heap.h | 12 ++++++++ > 2 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c > index c2ae19ba7d7e..8bb3c1876a69 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.c > +++ b/drivers/dma-buf/heaps/restricted_heap.c > @@ -12,10 +12,44 @@ > > #include "restricted_heap.h" > > +static int > +restricted_heap_memory_allocate(struct restricted_heap *rheap, struct restricted_buffer *buf) > +{ > + const struct restricted_heap_ops *ops = rheap->ops; > + int ret; > + > + ret = ops->alloc(rheap, buf); > + if (ret) > + return ret; > + > + if (ops->restrict_buf) { > + ret = ops->restrict_buf(rheap, buf); > + if (ret) > + goto buf_free; > + } > + return 0; > + > +buf_free: > + ops->free(rheap, buf); > + return ret; > +} > + > +static void > +restricted_heap_memory_free(struct restricted_heap *rheap, struct restricted_buffer *buf) > +{ > + const struct restricted_heap_ops *ops = rheap->ops; > + > + if (ops->unrestrict_buf) > + ops->unrestrict_buf(rheap, buf); > + > + ops->free(rheap, buf); > +} > + > static struct dma_buf * > restricted_heap_allocate(struct dma_heap *heap, unsigned long size, > unsigned long fd_flags, unsigned long heap_flags) > { > + struct restricted_heap *rheap = dma_heap_get_drvdata(heap); > struct restricted_buffer *restricted_buf; > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > struct dma_buf *dmabuf; > @@ -28,6 +62,9 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, > restricted_buf->size = ALIGN(size, PAGE_SIZE); > restricted_buf->heap = heap; > > + ret = restricted_heap_memory_allocate(rheap, restricted_buf); > + if (ret) > + goto err_free_buf; > exp_info.exp_name = dma_heap_get_name(heap); > exp_info.size = restricted_buf->size; > exp_info.flags = fd_flags; > @@ -36,11 +73,13 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, > dmabuf = dma_buf_export(&exp_info); > if (IS_ERR(dmabuf)) { > ret = PTR_ERR(dmabuf); > - goto err_free_buf; > + goto err_free_rstrd_mem; > } > > return dmabuf; > > +err_free_rstrd_mem: > + restricted_heap_memory_free(rheap, restricted_buf); > err_free_buf: > kfree(restricted_buf); > return ERR_PTR(ret); > diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h > index b448f77616ac..5783275d5714 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.h > +++ b/drivers/dma-buf/heaps/restricted_heap.h > @@ -15,6 +15,18 @@ struct restricted_buffer { > > struct restricted_heap { > const char *name; > + > + const struct restricted_heap_ops *ops; > +}; > + > +struct restricted_heap_ops { > + int (*heap_init)(struct restricted_heap *rheap); It might be worth moving this to a later patch when it's actually getting used. Thierry
On Wed, May 15, 2024 at 07:23:06PM GMT, Yong Wu wrote: > Add a MediaTek restricted heap which uses TEE service call to restrict > buffer. Currently this restricted heap is NULL, Prepare for the later > patch. Mainly there are two changes: > a) Add a heap_init ops since TEE probe late than restricted heap, thus > initialize the heap when we require the buffer the first time. > b) Add a priv_data for each heap, like the special data used by MTK > (such as "TEE session") can be placed in priv_data. > > Currently our heap depends on CMA which could only be bool, thus > depend on "TEE=y". > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/dma-buf/heaps/Kconfig | 7 ++ > drivers/dma-buf/heaps/Makefile | 1 + > drivers/dma-buf/heaps/restricted_heap.c | 11 ++ > drivers/dma-buf/heaps/restricted_heap.h | 2 + > drivers/dma-buf/heaps/restricted_heap_mtk.c | 115 ++++++++++++++++++++ > 5 files changed, 136 insertions(+) > create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index e54506f480ea..84f748fb2856 100644 > --- a/drivers/dma-buf/heaps/Kconfig > +++ b/drivers/dma-buf/heaps/Kconfig > @@ -21,3 +21,10 @@ config DMABUF_HEAPS_RESTRICTED > heap is to manage buffers that are inaccessible to the kernel and user space. > There may be several ways to restrict it, for example it may be encrypted or > protected by a TEE or hypervisor. If in doubt, say N. > + > +config DMABUF_HEAPS_RESTRICTED_MTK > + bool "MediaTek DMA-BUF Restricted Heap" > + depends on DMABUF_HEAPS_RESTRICTED && TEE=y > + help > + Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by > + TEE client interfaces. If in doubt, say N. > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile > index a2437c1817e2..0028aa9d875f 100644 > --- a/drivers/dma-buf/heaps/Makefile > +++ b/drivers/dma-buf/heaps/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o > obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED) += restricted_heap.o > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK) += restricted_heap_mtk.o > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o > diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c > index 4e45d46a6467..8bc8a5e3f969 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.c > +++ b/drivers/dma-buf/heaps/restricted_heap.c > @@ -151,11 +151,22 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, > unsigned long fd_flags, unsigned long heap_flags) > { > struct restricted_heap *rheap = dma_heap_get_drvdata(heap); > + const struct restricted_heap_ops *ops = rheap->ops; > struct restricted_buffer *restricted_buf; > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > struct dma_buf *dmabuf; > int ret; > > + /* > + * In some implements, TEE is required to protect buffer. However TEE probe > + * may be late, Thus heap_init is performed when the first buffer is requested. > + */ > + if (ops->heap_init) { > + ret = ops->heap_init(rheap); > + if (ret) > + return ERR_PTR(ret); > + } I wonder if we should make this parameterized rather than the default. Perhaps we can add a "init_on_demand" (or whatever other name) flag to struct restricted_heap_ops and then call this from heap initialization if possible and defer initialization depending on the restricted heap provider? > + > restricted_buf = kzalloc(sizeof(*restricted_buf), GFP_KERNEL); > if (!restricted_buf) > return ERR_PTR(-ENOMEM); > diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h > index 6d9599a4a34e..2a33a1c7a48b 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.h > +++ b/drivers/dma-buf/heaps/restricted_heap.h > @@ -19,6 +19,8 @@ struct restricted_heap { > const char *name; > > const struct restricted_heap_ops *ops; > + > + void *priv_data; Honestly, I would just get rid of any of this extra padding/indentation in these structures. There's really no benefit to this, except maybe if you *really* like things to be aligned, in which case the above is now probably worse than if you didn't try to align in the first place. Thierry
On Fri, Jun 28, 2024 at 01:47:01PM GMT, Thierry Reding wrote: > On Thu, Jun 27, 2024 at 04:40:02PM GMT, mripard@kernel.org wrote: > > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > > External email : Please do not click links or open attachments > > > > until > > > > > you have verified the sender or the content. > > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > > > I think it would be better to have the user configurable > > > > > private > > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > > requirement can get their private information from dma-buf > > > > > directly > > > > > > > > > and > > > > > > > > > no need to change or add the interface. > > > > > > > > > > > What's your opinion in this point? > > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > > secure > > > > > > > > device? > > > > > > > > > > We use the same mediatek-drm driver for secure and > > > > non-secure > > > > > buffer. > > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > > the > > > > > > > normal flow with normal hardware settings. > > > > > > > > > > We use different configurations to make hardware have > > > > different > > > > > > > permission to access the buffer it should access. > > > > > > > > > > So if we can't get the information of "the buffer is > > > > allocated > > > > > from > > > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > > > won't > > > > > > > be able to configure the hardware correctly. > > > > > > > > Why can't you get this information from userspace? > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > > kernel, it's just that for those gpus the render and kms node are the > > > > > same > > > > > driver so this is easy. > > > > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > So instead of this during buffer import: > > > > > > 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; > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > exchange or something like that. > > > > That doesn't work in all cases, unfortunately. > > > > If you're doing secure video playback, the firmware is typically in > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > aren't accessible by the CPU (or at least, not at the execution level > > Linux runs with). > > Can you clarify which firmware you're talking about? Is this secure > firmware, or firmware running on the video decoding hardware? Secure firmware > > So nobody can map that buffer, and the firmware driver is the one who > > knows that this buffer cannot be accessed by anyone. Putting this on the > > userspace to know would be pretty weird, and wouldn't solve the case > > where the kernel would try to map it. > > Doesn't userspace need to know from the start whether it's trying to do > secure playback or not? It does, but it won't know the capabilities of the buffer it gets back from the secure firmware. > Typically this involves more than just the decoding part. You'd > typically set up things like HDCP as part of the process, so userspace > probably already does know that the buffers being passed around are > protected. > > Also, the kernel shouldn't really be mapping these buffers unless > explicitly told to. In most cases you also wouldn't want the kernel to > map these kinds of buffers, right? Are there any specific cases where > you expect the kernel to need to map these? > > I've been looking at this on the Tegra side recently and the way it > works on these chips is that you basically get an opaque carveout region > that has been locked down by secure firmware or early bootloaders, so > only certain hardware blocks can access it. We can allocate from that > carveout and then pass the buffers around. So you allocate both the input and output buffers (and from different regions) from the application, and pass both to the secure firmware? Yeah, I guess that would work then. > It may be possible to use these protected carveout regions exclusively > from the DRM/KMS driver and share them with multimedia engines via DMA- > BUF, but I've also been looking into perhaps using DMA-BUF heaps to > expose the carveout, which would make this a bit more flexible and allow > either userspace to allocate the buffers or have multiple kernel drivers > share the carveout via the DMA-BUF heap. Though the latter would require > that there be in-kernel APIs for heaps, so not too sure about that yet. What would be the advantage of using a heap compared to having all these devices in DT use the reserved-memory property and point to that carveout? It should already work today. Maxime
On Fri, Jun 28, 2024 at 01:42:27PM GMT, Christian König wrote: > Am 27.06.24 um 16:40 schrieb mripard@kernel.org: > > [SNIP] > > > > > > > > Why can't you get this information from userspace? > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > > kernel, it's just that for those gpus the render and kms node are the > > > > > same > > > > > driver so this is easy. > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > So instead of this during buffer import: > > > > > > 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; > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > exchange or something like that. > > That doesn't work in all cases, unfortunately. > > > > If you're doing secure video playback, the firmware is typically in > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > aren't accessible by the CPU (or at least, not at the execution level > > Linux runs with). > > Yeah, that's perfectly fine. At least the AMD encryption solution > works exactly like that as well. > > So nobody can map that buffer, and the firmware driver is the one who > > knows that this buffer cannot be accessed by anyone. > > On most hw I know you can actually map that buffer, it's just that the > CPU sees only garbage in it because you don't have the necessary > decryption keys around. So you can always map and access the buffer, but only if you're in the right "context" the content would be correct? I think that part is pretty different than what ARM SoCs are doing, where they would typically prevent any CPU access and fault on access. > > Putting this on the userspace to know would be pretty weird, and > > wouldn't solve the case where the kernel would try to map it. > > But that's exactly how all other implementations work as far as I know. I > mean what do you do if the kernel maps the encrypted buffer? > > On AMD we also block userspace and kernel CPU accesses, but that is only > done to make it easier to find bugs not for correctness. > > And userspace absolutely needs to be aware that a buffer is encrypted, cause > otherwise it could potentially try to access it with the CPU. I absolutely agree. I guess our discussion is whether it's something that should be implicit and understood by applications, or if it should be explicit and discoverable. Maxime
On Fri, Jun 28, 2024 at 02:34:24PM GMT, Christian König wrote: > Am 28.06.24 um 13:47 schrieb Thierry Reding: > > [SNIP] > > > > The reason I ask is that encryption here looks just like another parameter > > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > > > So instead of this during buffer import: > > > > > > > > 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; > > > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > > exchange or something like that. > > > That doesn't work in all cases, unfortunately. > > > > > > If you're doing secure video playback, the firmware is typically in > > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > > aren't accessible by the CPU (or at least, not at the execution level > > > Linux runs with). > > Can you clarify which firmware you're talking about? Is this secure > > firmware, or firmware running on the video decoding hardware? > > > > > So nobody can map that buffer, and the firmware driver is the one who > > > knows that this buffer cannot be accessed by anyone. Putting this on the > > > userspace to know would be pretty weird, and wouldn't solve the case > > > where the kernel would try to map it. > > Doesn't userspace need to know from the start whether it's trying to do > > secure playback or not? Typically this involves more than just the > > decoding part. You'd typically set up things like HDCP as part of the > > process, so userspace probably already does know that the buffers being > > passed around are protected. > > > > Also, the kernel shouldn't really be mapping these buffers unless > > explicitly told to. In most cases you also wouldn't want the kernel to > > map these kinds of buffers, right? Are there any specific cases where > > you expect the kernel to need to map these? > > > > I've been looking at this on the Tegra side recently and the way it > > works on these chips is that you basically get an opaque carveout region > > that has been locked down by secure firmware or early bootloaders, so > > only certain hardware blocks can access it. We can allocate from that > > carveout and then pass the buffers around. > > > > It may be possible to use these protected carveout regions exclusively > > from the DRM/KMS driver and share them with multimedia engines via DMA- > > BUF, but I've also been looking into perhaps using DMA-BUF heaps to > > expose the carveout, which would make this a bit more flexible and allow > > either userspace to allocate the buffers or have multiple kernel drivers > > share the carveout via the DMA-BUF heap. Though the latter would require > > that there be in-kernel APIs for heaps, so not too sure about that yet. > > Yeah as far as I can see that would be a perfectly valid use case for > DMA-Buf heaps. > > One question here: How does the HDCP setup work on Tegra? From your comment > I guess you pass most of the information through userspace as well. Well, we don't currently support HDCP at all. I do have proof-of-concept patches from a long time ago and, yes, judging by that we'd need to control all of this from userspace. The way I imagine that this would work is that userspace needs to first set the "Content Protection" and "HDCP Content Type" properties and wait for the state change. Once that has happened it can go and allocate the protected memory and start decoding into it and then scan out from these buffers. > Or is there any info inside the DMA-buf for this? In other words would you > also need to know if a buffer is then allocated from this special carveout? I don't think so. It's possible to scan out an unprotected buffer with HDCP enabled. It may also be possible to scan out a protected buffer even if HDCP wasn't enabled, though you would obviously want to prevent that somehow. Not sure if there's a common way to do this, but I guess in end-user devices you'd need a fully trusted boot chain to do that in a compliant way. It's been a long time since I looked at this, but I seem to recall that at the time all software that could do DRM-protected playback on Linux was proprietary for reasons like these. Thierry
On Fri, Jun 28, 2024 at 03:21:51PM GMT, mripard@kernel.org wrote: > On Fri, Jun 28, 2024 at 01:47:01PM GMT, Thierry Reding wrote: > > On Thu, Jun 27, 2024 at 04:40:02PM GMT, mripard@kernel.org wrote: > > > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > > > External email : Please do not click links or open attachments > > > > > until > > > > > > you have verified the sender or the content. > > > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > > > > I think it would be better to have the user configurable > > > > > > private > > > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > > > requirement can get their private information from dma-buf > > > > > > directly > > > > > > > > > > and > > > > > > > > > > no need to change or add the interface. > > > > > > > > > > > > What's your opinion in this point? > > > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > > > secure > > > > > > > > > device? > > > > > > > > > > > We use the same mediatek-drm driver for secure and > > > > > non-secure > > > > > > buffer. > > > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > > > the > > > > > > > > normal flow with normal hardware settings. > > > > > > > > > > > We use different configurations to make hardware have > > > > > different > > > > > > > > permission to access the buffer it should access. > > > > > > > > > > > So if we can't get the information of "the buffer is > > > > > allocated > > > > > > from > > > > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > > > > won't > > > > > > > > be able to configure the hardware correctly. > > > > > > > > > Why can't you get this information from userspace? > > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > > > kernel, it's just that for those gpus the render and kms node are the > > > > > > same > > > > > > driver so this is easy. > > > > > > > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > > > So instead of this during buffer import: > > > > > > > > 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; > > > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > > exchange or something like that. > > > > > > That doesn't work in all cases, unfortunately. > > > > > > If you're doing secure video playback, the firmware is typically in > > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > > aren't accessible by the CPU (or at least, not at the execution level > > > Linux runs with). > > > > Can you clarify which firmware you're talking about? Is this secure > > firmware, or firmware running on the video decoding hardware? > > Secure firmware Ah... interesting. So you actually need to interop with that firmware in order to start decryption/decoding. That's quite different from how this works on Tegra. Well, maybe not entirely. For Tegra there is firmware that runs on the hardware decoder and which has access to the keys, so in that way I guess it's similar to your use-case, except the firmware runs on a different chip. > > > So nobody can map that buffer, and the firmware driver is the one who > > > knows that this buffer cannot be accessed by anyone. Putting this on the > > > userspace to know would be pretty weird, and wouldn't solve the case > > > where the kernel would try to map it. > > > > Doesn't userspace need to know from the start whether it's trying to do > > secure playback or not? > > It does, but it won't know the capabilities of the buffer it gets back > from the secure firmware. I think that's kind of the point. Does it really have to know the capabilities? Isn't it enough to know that it's got some sort of protected buffer back and then use it more or less blindly? I mean these are things that have to be tightly coupled no matter what, so how much point is there in trying to validate what you get? > > Typically this involves more than just the decoding part. You'd > > typically set up things like HDCP as part of the process, so userspace > > probably already does know that the buffers being passed around are > > protected. > > > > Also, the kernel shouldn't really be mapping these buffers unless > > explicitly told to. In most cases you also wouldn't want the kernel to > > map these kinds of buffers, right? Are there any specific cases where > > you expect the kernel to need to map these? > > > > I've been looking at this on the Tegra side recently and the way it > > works on these chips is that you basically get an opaque carveout region > > that has been locked down by secure firmware or early bootloaders, so > > only certain hardware blocks can access it. We can allocate from that > > carveout and then pass the buffers around. > > So you allocate both the input and output buffers (and from different > regions) from the application, and pass both to the secure firmware? > > Yeah, I guess that would work then. It doesn't really matter who allocates the buffers. It could be the application allocating the scanout buffer from a DRM/KMS device and the input buffer from the multimedia decoder. Or it could be the application allocating both buffers from different DMA-BUF heaps. In the end it shouldn't really matter where they are coming from. It's effectively up to the application to pass the right buffers into the right IOCTLs. > > It may be possible to use these protected carveout regions exclusively > > from the DRM/KMS driver and share them with multimedia engines via DMA- > > BUF, but I've also been looking into perhaps using DMA-BUF heaps to > > expose the carveout, which would make this a bit more flexible and allow > > either userspace to allocate the buffers or have multiple kernel drivers > > share the carveout via the DMA-BUF heap. Though the latter would require > > that there be in-kernel APIs for heaps, so not too sure about that yet. > > What would be the advantage of using a heap compared to having all these > devices in DT use the reserved-memory property and point to that > carveout? It should already work today. You can't just have all of these point to a common reserved-memory node because there can be multiple concurrent users. You could have multiple protected streams running at the same time. DMA-BUF heaps allows us to expose a central provider for the protected memory so that allocations can be properly arbitrated. Thierry
On Fri, Jun 28, 2024 at 03:57:49PM +0200, Thierry Reding wrote: > On Fri, Jun 28, 2024 at 02:34:24PM GMT, Christian König wrote: > > Am 28.06.24 um 13:47 schrieb Thierry Reding: > > > [SNIP] > > > > > The reason I ask is that encryption here looks just like another parameter > > > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > > > > > So instead of this during buffer import: > > > > > > > > > > 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; > > > > > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > > > exchange or something like that. > > > > That doesn't work in all cases, unfortunately. > > > > > > > > If you're doing secure video playback, the firmware is typically in > > > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > > > aren't accessible by the CPU (or at least, not at the execution level > > > > Linux runs with). > > > Can you clarify which firmware you're talking about? Is this secure > > > firmware, or firmware running on the video decoding hardware? > > > > > > > So nobody can map that buffer, and the firmware driver is the one who > > > > knows that this buffer cannot be accessed by anyone. Putting this on the > > > > userspace to know would be pretty weird, and wouldn't solve the case > > > > where the kernel would try to map it. > > > Doesn't userspace need to know from the start whether it's trying to do > > > secure playback or not? Typically this involves more than just the > > > decoding part. You'd typically set up things like HDCP as part of the > > > process, so userspace probably already does know that the buffers being > > > passed around are protected. > > > > > > Also, the kernel shouldn't really be mapping these buffers unless > > > explicitly told to. In most cases you also wouldn't want the kernel to > > > map these kinds of buffers, right? Are there any specific cases where > > > you expect the kernel to need to map these? > > > > > > I've been looking at this on the Tegra side recently and the way it > > > works on these chips is that you basically get an opaque carveout region > > > that has been locked down by secure firmware or early bootloaders, so > > > only certain hardware blocks can access it. We can allocate from that > > > carveout and then pass the buffers around. > > > > > > It may be possible to use these protected carveout regions exclusively > > > from the DRM/KMS driver and share them with multimedia engines via DMA- > > > BUF, but I've also been looking into perhaps using DMA-BUF heaps to > > > expose the carveout, which would make this a bit more flexible and allow > > > either userspace to allocate the buffers or have multiple kernel drivers > > > share the carveout via the DMA-BUF heap. Though the latter would require > > > that there be in-kernel APIs for heaps, so not too sure about that yet. > > > > Yeah as far as I can see that would be a perfectly valid use case for > > DMA-Buf heaps. > > > > One question here: How does the HDCP setup work on Tegra? From your comment > > I guess you pass most of the information through userspace as well. > > Well, we don't currently support HDCP at all. I do have proof-of-concept > patches from a long time ago and, yes, judging by that we'd need to > control all of this from userspace. The way I imagine that this would > work is that userspace needs to first set the "Content Protection" and > "HDCP Content Type" properties and wait for the state change. Once that > has happened it can go and allocate the protected memory and start > decoding into it and then scan out from these buffers. Yeah this is how this is meant to work, because some userspace is happy with just hdcp, while the buffers are not encrypted/protected, the only protection is system lockdown with secure boot. So enable hdcp first, then allocate secure buffers and display them there. And the hardware needs to make sure that if we ever drop hcpd, the screen goes black (or encrypted garbage) when trying to display encrypted/secure buffers. > > Or is there any info inside the DMA-buf for this? In other words would you > > also need to know if a buffer is then allocated from this special carveout? > > I don't think so. It's possible to scan out an unprotected buffer with > HDCP enabled. It may also be possible to scan out a protected buffer > even if HDCP wasn't enabled, though you would obviously want to prevent > that somehow. Not sure if there's a common way to do this, but I guess > in end-user devices you'd need a fully trusted boot chain to do that in > a compliant way. Where I know how it works it's hardware making that guarantee. > It's been a long time since I looked at this, but I seem to recall that > at the time all software that could do DRM-protected playback on Linux > was proprietary for reasons like these. Yeah it's still not much better unfortunately :-/ -Sima
Hi Christian, Le jeudi 27 juin 2024 à 08:57 +0200, Christian König a écrit : > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > > External email : Please do not click links or open attachments until > > > you have verified the sender or the content. > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > requirement can get their private information from dma-buf > > > directly > > > > > > > and > > > > > > > no need to change or add the interface. > > > > > > > > > What's your opinion in this point? > > > > > > > Well of hand I don't see the need for that. > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > > > device? > > > > > > > > > > We use the same mediatek-drm driver for secure and non-secure > > > buffer. > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > > > normal flow with normal hardware settings. > > > > > > > > > > We use different configurations to make hardware have different > > > > > permission to access the buffer it should access. > > > > > > > > > > So if we can't get the information of "the buffer is allocated > > > from > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > > > be able to configure the hardware correctly. > > > > > > > > Why can't you get this information from userspace? > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > kernel, it's just that for those gpus the render and kms node are the > > > same > > > driver so this is easy. > > > > > The reason I ask is that encryption here looks just like another > parameter for the buffer, e.g. like format, stride, tilling etc.. I'm mostly a reader of the thread here, but I'd like to avoid basic mistakes. The buffer in question are "protected", meaning that the CPU HW does not have access to the underlying pages (or zone in the case of Meditatek). This is different from encrypted buffers, which don't need this level of protection, as without the security key to decrypt them, their content is close to random data. > > So instead of this during buffer import: > > 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; > > You can trivially say during use hey this buffer is encrypted. > > At least that's my 10 mile high view, maybe I'm missing some extensive > key exchange or something like that. If we take secure video path as an example, in the context of digital right management, the handling of user session, retrieval of the device specific "key" is entirely something for userspace to handle and the kernel have no business into that. As long as the data is encrypted, its safe to carry around like any other buffers. It is only once decryption (usally done by a TF-A) that restricted memory start being used. Initially in the form of a compressed video stream, and eventually in the format of raw images. > > > > > But on arm you have split designs everywhere and dma-buf > > > import/export, so > > > something else is needed. And neither current kms uapi nor > > > protocols/extensions have provisions for this (afaik) because it > > > works on > > > the big gpus, and on android it's just hacked up with backchannels. > > > > > > So yeah essentially I think we probably need something like this, as > > > much > > > as it sucks. I see it somewhat similar to handling pcip2pdma > > > limitations > > > in the kernel too. > > > > > > Not sure where/how it should be handled though, and maybe I've missed > > > something around protocols, in which case I guess we should add some > > > secure buffer flags to the ADDFB2 ioctl. > > > > Thanks for your hint, I'll try to add the secure flag to the ADDFB2 > > ioctl. If it works, I'll send the patch. > > Yeah, exactly what I would suggest as well. > > I'm not an expert for that part, but as far as I know we already have > bunch of device specific tilling flags in there. > > Adding an MTK_ENCRYPTED flag should be trivial. Just to be clear, my comment was just a concept correction, I also think its nice to give a ADDFB2 flag a try, from my userspace experience, this is an easy place to provide this type of information. Also, the V4L2 proposal for the same endup with a flag at during buffer queue configuration, which is pretty close. Nicolas > > Regards, > Christian. > > > > > Regards, > > Jason-JH.Lin > > > > > -Sima > > > > ************* MEDIATEK Confidentiality Notice ******************** > > The information contained in this e-mail message (including any > > attachments) may be confidential, proprietary, privileged, or otherwise > > exempt from disclosure under applicable laws. It is intended to be > > conveyed only to the designated recipient(s). Any use, dissemination, > > distribution, printing, retaining or copying of this e-mail (including its > > attachments) by unintended recipient(s) is strictly prohibited and may > > be unlawful. If you are not an intended recipient of this e-mail, or believe > > that you have received this e-mail in error, please notify the sender > > immediately (by replying to this e-mail), delete any and all copies of > > this e-mail (including any attachments) from your system, and do not > > disclose the content of this e-mail to any other person. Thank you! > >
Hi, Le jeudi 27 juin 2024 à 16:40 +0200, mripard@kernel.org a écrit : > > You can trivially say during use hey this buffer is encrypted. > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > exchange or something like that. > > That doesn't work in all cases, unfortunately. > > If you're doing secure video playback, the firmware is typically in > charge of the frame decryption/decoding, and you'd get dma-buf back that > aren't accessible by the CPU (or at least, not at the execution level > Linux runs with). > > So nobody can map that buffer, and the firmware driver is the one who > knows that this buffer cannot be accessed by anyone. Putting this on the > userspace to know would be pretty weird, and wouldn't solve the case > where the kernel would try to map it. Userspace will be the one calling into the CDM TF-A to get the bitstream buffer to be decrypted, not the firmware. The encrypted buffers are not using restricted memory. Userspace is also responsible for calling into the MTK restricted heap to allocate the destination buffer (on other platform it could be CMA heaps + TF-A call to restrict the allocated memory, I've seen some discussions related to this, but its not possible on Mediatek HW). I think its fair to assume that userspace always know which buffers are restricted or not in the SVP process. Nicolas
Hi Thierry, Le vendredi 28 juin 2024 à 16:11 +0200, Thierry Reding a écrit : > On Fri, Jun 28, 2024 at 03:21:51PM GMT, mripard@kernel.org wrote: > > On Fri, Jun 28, 2024 at 01:47:01PM GMT, Thierry Reding wrote: > > > On Thu, Jun 27, 2024 at 04:40:02PM GMT, mripard@kernel.org wrote: > > > > On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > > > > > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > > > > > External email : Please do not click links or open attachments > > > > > > until > > > > > > > you have verified the sender or the content. > > > > > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > > > > > I think it would be better to have the user configurable > > > > > > > private > > > > > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > > > > > requirement can get their private information from dma-buf > > > > > > > directly > > > > > > > > > > > and > > > > > > > > > > > no need to change or add the interface. > > > > > > > > > > > > > What's your opinion in this point? > > > > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > > > > What happens if you get a non-secure buffer imported in your > > > > > > > secure > > > > > > > > > > device? > > > > > > > > > > > > We use the same mediatek-drm driver for secure and > > > > > > non-secure > > > > > > > buffer. > > > > > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > > > > > the > > > > > > > > > normal flow with normal hardware settings. > > > > > > > > > > > > We use different configurations to make hardware have > > > > > > different > > > > > > > > > permission to access the buffer it should access. > > > > > > > > > > > > So if we can't get the information of "the buffer is > > > > > > allocated > > > > > > > from > > > > > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > > > > > won't > > > > > > > > > be able to configure the hardware correctly. > > > > > > > > > > Why can't you get this information from userspace? > > > > > > > > Same reason amd and i915/xe also pass this around internally in the > > > > > > > kernel, it's just that for those gpus the render and kms node are the > > > > > > > same > > > > > > > driver so this is easy. > > > > > > > > > > > > > > > > > The reason I ask is that encryption here looks just like another parameter > > > > > for the buffer, e.g. like format, stride, tilling etc.. > > > > > > > > > > So instead of this during buffer import: > > > > > > > > > > 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; > > > > > > > > > > You can trivially say during use hey this buffer is encrypted. > > > > > > > > > > At least that's my 10 mile high view, maybe I'm missing some extensive key > > > > > exchange or something like that. > > > > > > > > That doesn't work in all cases, unfortunately. > > > > > > > > If you're doing secure video playback, the firmware is typically in > > > > charge of the frame decryption/decoding, and you'd get dma-buf back that > > > > aren't accessible by the CPU (or at least, not at the execution level > > > > Linux runs with). > > > > > > Can you clarify which firmware you're talking about? Is this secure > > > firmware, or firmware running on the video decoding hardware? > > > > Secure firmware > > Ah... interesting. So you actually need to interop with that firmware in > order to start decryption/decoding. That's quite different from how this > works on Tegra. Well, maybe not entirely. For Tegra there is firmware > that runs on the hardware decoder and which has access to the keys, so > in that way I guess it's similar to your use-case, except the firmware > runs on a different chip. That is something interesting for the linux-media discussions too. So in one case, you have a seperate TF-A in the secure firmware following the CDM specification, and it gives you back a restricted bitstream buffer. You then don't need any CDM specific session/key information into the CODEC driver. But in the case of Tegra, it would mean the CODEC driver is not agnostic to the CDM, so we can expect (if this endup as a V4L2 driver) some controls for Widewine, Playready and other CDM ? (adding explicit CDM API in the kernel is a hot potato imho, I myself would try and stay away from that at all cost, and focus on restricted storage feature only). Nicolas > > > > So nobody can map that buffer, and the firmware driver is the one who > > > > knows that this buffer cannot be accessed by anyone. Putting this on the > > > > userspace to know would be pretty weird, and wouldn't solve the case > > > > where the kernel would try to map it. > > > > > > Doesn't userspace need to know from the start whether it's trying to do > > > secure playback or not? > > > > It does, but it won't know the capabilities of the buffer it gets back > > from the secure firmware. > > I think that's kind of the point. Does it really have to know the > capabilities? Isn't it enough to know that it's got some sort of > protected buffer back and then use it more or less blindly? I mean > these are things that have to be tightly coupled no matter what, so > how much point is there in trying to validate what you get? > > > > Typically this involves more than just the decoding part. You'd > > > typically set up things like HDCP as part of the process, so userspace > > > probably already does know that the buffers being passed around are > > > protected. > > > > > > Also, the kernel shouldn't really be mapping these buffers unless > > > explicitly told to. In most cases you also wouldn't want the kernel to > > > map these kinds of buffers, right? Are there any specific cases where > > > you expect the kernel to need to map these? > > > > > > I've been looking at this on the Tegra side recently and the way it > > > works on these chips is that you basically get an opaque carveout region > > > that has been locked down by secure firmware or early bootloaders, so > > > only certain hardware blocks can access it. We can allocate from that > > > carveout and then pass the buffers around. > > > > So you allocate both the input and output buffers (and from different > > regions) from the application, and pass both to the secure firmware? > > > > Yeah, I guess that would work then. > > It doesn't really matter who allocates the buffers. It could be the > application allocating the scanout buffer from a DRM/KMS device and the > input buffer from the multimedia decoder. Or it could be the application > allocating both buffers from different DMA-BUF heaps. In the end it > shouldn't really matter where they are coming from. It's effectively up > to the application to pass the right buffers into the right IOCTLs. > > > > It may be possible to use these protected carveout regions exclusively > > > from the DRM/KMS driver and share them with multimedia engines via DMA- > > > BUF, but I've also been looking into perhaps using DMA-BUF heaps to > > > expose the carveout, which would make this a bit more flexible and allow > > > either userspace to allocate the buffers or have multiple kernel drivers > > > share the carveout via the DMA-BUF heap. Though the latter would require > > > that there be in-kernel APIs for heaps, so not too sure about that yet. > > > > What would be the advantage of using a heap compared to having all these > > devices in DT use the reserved-memory property and point to that > > carveout? It should already work today. > > You can't just have all of these point to a common reserved-memory node > because there can be multiple concurrent users. You could have multiple > protected streams running at the same time. DMA-BUF heaps allows us to > expose a central provider for the protected memory so that allocations > can be properly arbitrated. > > Thierry
Am 28.06.24 um 22:16 schrieb Nicolas Dufresne: > [SNIP] >>>>> Why can't you get this information from userspace? >>>> Same reason amd and i915/xe also pass this around internally in the >>>> kernel, it's just that for those gpus the render and kms node are the >>>> same >>>> driver so this is easy. >>>> >> The reason I ask is that encryption here looks just like another >> parameter for the buffer, e.g. like format, stride, tilling etc.. > I'm mostly a reader of the thread here, but I'd like to avoid basic mistakes. > The buffer in question are "protected", meaning that the CPU HW does not have > access to the underlying pages (or zone in the case of Meditatek). > > This is different from encrypted buffers, which don't need this level of > protection, as without the security key to decrypt them, their content is close > to random data. Thanks for that clarification, this difference was absolutely not obvious. In that case having a separate heap for this memory is indeed the easiest approach. My question is still what would happen if the CPU tries to access this protected buffer? Or does the CPU not even have an address to do that? Just out of curiosity, I mean the exporting heap should then somehow reject any attempt to mmap() or vmap() the buffer content. Thanks, Christian.
On Mon, 2024-07-01 at 11:14 +0200, Christian König wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Am 28.06.24 um 15:40 schrieb mripard@kernel.org: > > [SNIP] > > > > So nobody can map that buffer, and the firmware driver is the > > > > one who > > > > knows that this buffer cannot be accessed by anyone. > > > > > > On most hw I know you can actually map that buffer, it's just > > > that the > > > CPU sees only garbage in it because you don't have the necessary > > > decryption keys around. > > > > So you can always map and access the buffer, but only if you're in > > the > > right "context" the content would be correct? > > Exactly that, yes. You need to have access to the decryption keys. > > > I think that part is pretty different than what ARM SoCs are doing, > > where they would typically prevent any CPU access and fault on > > access. > > Yeah, that's indeed an important difference Nicolas noted as well. > > As I know, there are 4 kind of buffer in SVP flow of MediaTek platform: 1) The normal buffer with the encrypted video bitstream content (source buffer for decryption engine) User APP will push encrypted data to this buffer for decrypted engine, so it should be normal buffer. 2) The secure buffer with the decrypted video bitstream content (source buffer for video decoder engine) 3) The secure buffer with decoded frame data content (source buffer for GPU) 4) The secure buffer with resized and format changed frame data content (source buffer for display engine) The secure buffer in DRM SVP series: https://patchwork.kernel.org/project/linux-mediatek/list/?series=855888 should be resized and format changed frame data for display engine and it can only be accessed in TEE world by the secure display hardware. If CPU access this buffer in normal world or non-secure display hardware are configured to access this buffer, they will get an IOMMU fault error and it can not translate to the correct buffer address. > > > > Putting this on the userspace to know would be pretty weird, > > > > and > > > > wouldn't solve the case where the kernel would try to map it. > > > > > > But that's exactly how all other implementations work as far as I > > > know. I > > > mean what do you do if the kernel maps the encrypted buffer? > > > > > > On AMD we also block userspace and kernel CPU accesses, but that > > > is only > > > done to make it easier to find bugs not for correctness. > > > > > > And userspace absolutely needs to be aware that a buffer is > > > encrypted, cause > > > otherwise it could potentially try to access it with the CPU. > > > > I absolutely agree. I guess our discussion is whether it's > > something > > that should be implicit and understood by applications, or if it > > should > > be explicit and discoverable. > > Oh good point as well. But I think that's more a question for the > userspace stack design. > > E.g. it can be handled explicitly by the application itself or > implicitly by some V4L or VA-API library or something similar. > > For the kernel UAPI design we agreed at some point that we don't want > to have any implicit properties on the DMA-buf which are carried > around by the kernel, e.g. the whole format metadata for example. > I've asked our vcodec owner how to handle the same case in V4L2: https://patchwork.kernel.org/project/linux-mediatek/patch/20240516122102.16379-2-yunfei.dong@mediatek.com/ They also added a secure flag in UAPI and V4L2 header and then passed it from userspace to V4L2 driver. So maybe we can use the same way to add a secure flag in UAPI and extend the flag parameter of AddModeFB2(). I think here is currently the most suitable place to add the secure flag besides adding the new ioctl(). Regards, Jason-JH.Lin > One notable exception to this is the actual hw topology, e.g. when > for example a device has a special interconnect to another device and > you need to make sure that the devices are powered up and down in a > specific order to make things work. > > This should then made known to the core kernel using device link > structure. E.g. similar to how it's used between GPU and HDMI audio > block for example. > > Regards, > Christian. > > > Maxime >
On Wed, May 15, 2024 at 1:25 PM Yong Wu <yong.wu@mediatek.com> wrote: > > Add a MediaTek restricted heap which uses TEE service call to restrict > buffer. Currently this restricted heap is NULL, Prepare for the later > patch. Mainly there are two changes: > a) Add a heap_init ops since TEE probe late than restricted heap, thus > initialize the heap when we require the buffer the first time. > b) Add a priv_data for each heap, like the special data used by MTK > (such as "TEE session") can be placed in priv_data. > > Currently our heap depends on CMA which could only be bool, thus > depend on "TEE=y". > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/dma-buf/heaps/Kconfig | 7 ++ > drivers/dma-buf/heaps/Makefile | 1 + > drivers/dma-buf/heaps/restricted_heap.c | 11 ++ > drivers/dma-buf/heaps/restricted_heap.h | 2 + > drivers/dma-buf/heaps/restricted_heap_mtk.c | 115 ++++++++++++++++++++ > 5 files changed, 136 insertions(+) > create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c > > diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig > index e54506f480ea..84f748fb2856 100644 > --- a/drivers/dma-buf/heaps/Kconfig > +++ b/drivers/dma-buf/heaps/Kconfig > @@ -21,3 +21,10 @@ config DMABUF_HEAPS_RESTRICTED > heap is to manage buffers that are inaccessible to the kernel and user space. > There may be several ways to restrict it, for example it may be encrypted or > protected by a TEE or hypervisor. If in doubt, say N. > + > +config DMABUF_HEAPS_RESTRICTED_MTK > + bool "MediaTek DMA-BUF Restricted Heap" > + depends on DMABUF_HEAPS_RESTRICTED && TEE=y > + help > + Enable restricted dma-buf heaps for MediaTek platform. This heap is backed by > + TEE client interfaces. If in doubt, say N. > diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile > index a2437c1817e2..0028aa9d875f 100644 > --- a/drivers/dma-buf/heaps/Makefile > +++ b/drivers/dma-buf/heaps/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o > obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED) += restricted_heap.o > +obj-$(CONFIG_DMABUF_HEAPS_RESTRICTED_MTK) += restricted_heap_mtk.o > obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o > diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c > index 4e45d46a6467..8bc8a5e3f969 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.c > +++ b/drivers/dma-buf/heaps/restricted_heap.c > @@ -151,11 +151,22 @@ restricted_heap_allocate(struct dma_heap *heap, unsigned long size, > unsigned long fd_flags, unsigned long heap_flags) > { > struct restricted_heap *rheap = dma_heap_get_drvdata(heap); > + const struct restricted_heap_ops *ops = rheap->ops; > struct restricted_buffer *restricted_buf; > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > struct dma_buf *dmabuf; > int ret; > > + /* > + * In some implements, TEE is required to protect buffer. However TEE probe > + * may be late, Thus heap_init is performed when the first buffer is requested. > + */ > + if (ops->heap_init) { > + ret = ops->heap_init(rheap); > + if (ret) > + return ERR_PTR(ret); > + } > + > restricted_buf = kzalloc(sizeof(*restricted_buf), GFP_KERNEL); > if (!restricted_buf) > return ERR_PTR(-ENOMEM); > diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h > index 6d9599a4a34e..2a33a1c7a48b 100644 > --- a/drivers/dma-buf/heaps/restricted_heap.h > +++ b/drivers/dma-buf/heaps/restricted_heap.h > @@ -19,6 +19,8 @@ struct restricted_heap { > const char *name; > > const struct restricted_heap_ops *ops; > + > + void *priv_data; > }; > > struct restricted_heap_ops { > diff --git a/drivers/dma-buf/heaps/restricted_heap_mtk.c b/drivers/dma-buf/heaps/restricted_heap_mtk.c > new file mode 100644 > index 000000000000..52e805eb9858 > --- /dev/null > +++ b/drivers/dma-buf/heaps/restricted_heap_mtk.c > @@ -0,0 +1,115 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * DMABUF restricted heap exporter for MediaTek > + * > + * Copyright (C) 2024 MediaTek Inc. > + */ > +#define pr_fmt(fmt) "rheap_mtk: " fmt > + > +#include <linux/dma-buf.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/tee_drv.h> > +#include <linux/uuid.h> > + > +#include "restricted_heap.h" > + > +#define TZ_TA_MEM_UUID_MTK "4477588a-8476-11e2-ad15-e41f1390d676" > + > +#define TEE_PARAM_NUM 4 > + > +enum mtk_secure_mem_type { > + /* > + * MediaTek static chunk memory carved out for TrustZone. The memory > + * management is inside the TEE. > + */ > + MTK_SECURE_MEMORY_TYPE_CM_TZ = 1, > +}; > + > +struct mtk_restricted_heap_data { > + struct tee_context *tee_ctx; > + u32 tee_session; > + > + const enum mtk_secure_mem_type mem_type; > + > +}; > + > +static int mtk_tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data) > +{ > + return ver->impl_id == TEE_IMPL_ID_OPTEE; > +} > + > +static int mtk_tee_session_init(struct mtk_restricted_heap_data *data) > +{ > + struct tee_param t_param[TEE_PARAM_NUM] = {0}; > + struct tee_ioctl_open_session_arg arg = {0}; > + uuid_t ta_mem_uuid; > + int ret; > + > + data->tee_ctx = tee_client_open_context(NULL, mtk_tee_ctx_match, NULL, NULL); > + if (IS_ERR(data->tee_ctx)) { > + pr_err_once("%s: open context failed, ret=%ld\n", __func__, > + PTR_ERR(data->tee_ctx)); > + return -ENODEV; > + } > + > + arg.num_params = TEE_PARAM_NUM; > + arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC; > + ret = uuid_parse(TZ_TA_MEM_UUID_MTK, &ta_mem_uuid); > + if (ret) > + goto close_context; > + memcpy(&arg.uuid, &ta_mem_uuid.b, sizeof(ta_mem_uuid)); > + > + ret = tee_client_open_session(data->tee_ctx, &arg, t_param); > + if (ret < 0 || arg.ret) { > + pr_err_once("%s: open session failed, ret=%d:%d\n", > + __func__, ret, arg.ret); > + ret = -EINVAL; > + goto close_context; > + } > + data->tee_session = arg.session; > + return 0; > + > +close_context: > + tee_client_close_context(data->tee_ctx); There's a data->tee_ctx = NULL; missing here. Cheers, Jens > + return ret; > +} > + > +static int mtk_restricted_heap_init(struct restricted_heap *rheap) > +{ > + struct mtk_restricted_heap_data *data = rheap->priv_data; > + > + if (!data->tee_ctx) > + return mtk_tee_session_init(data); > + return 0; > +} > + > +static const struct restricted_heap_ops mtk_restricted_heap_ops = { > + .heap_init = mtk_restricted_heap_init, > +}; > + > +static struct mtk_restricted_heap_data mtk_restricted_heap_data = { > + .mem_type = MTK_SECURE_MEMORY_TYPE_CM_TZ, > +}; > + > +static struct restricted_heap mtk_restricted_heaps[] = { > + { > + .name = "restricted_mtk_cm", > + .ops = &mtk_restricted_heap_ops, > + .priv_data = &mtk_restricted_heap_data, > + }, > +}; > + > +static int mtk_restricted_heap_initialize(void) > +{ > + struct restricted_heap *rheap = mtk_restricted_heaps; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(mtk_restricted_heaps); i++, rheap++) > + restricted_heap_add(rheap); > + return 0; > +} > +module_init(mtk_restricted_heap_initialize); > +MODULE_DESCRIPTION("MediaTek Restricted Heap Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.25.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The purpose of this patchset is for MediaTek secure video playback, and also to enable other potential uses of this in the future. The 'restricted dma-heap' will be used to allocate dma_buf objects that reference memory in the secure world that is inaccessible/unmappable by the non-secure (i.e. kernel/userspace) world. That memory will be used by the secure/ trusted world to store secure information (i.e. decrypted media content). The dma_bufs allocated from the kernel will be passed to V4L2 for video decoding (as input and output). They will also be used by the drm system for rendering of the content. This patchset adds two MediaTek restricted heaps and they will be used in v4l2[1] and drm[2]. 1) restricted_mtk_cm: secure chunk memory for MediaTek SVP (Secure Video Path). The buffer is reserved for the secure world after bootup and it is used for vcodec's ES/working buffer; 2) restricted_mtk_cma: secure CMA memory for MediaTek SVP. This buffer is dynamically reserved for the secure world and will be got when we start playing secure videos. Once the security video playing is complete, the CMA will be released. This heap is used for the vcodec's frame buffer. [1] https://lore.kernel.org/linux-mediatek/20240412090851.24999-1-yunfei.dong@mediatek.com/ [2] https://lore.kernel.org/linux-mediatek/20240403102701.369-1-shawn.sung@mediatek.com/ Change note: v5: 1) Reconstruct TEE commands to allow the kernel to obtain the PA of the TEE buffer to initialize a valid sg table. 2) Previously, PA was hidden from the kernel. Then the kernel checks if this is restricted buffer by "if (sg_page(sg) == NULL)". In this version, we will add a new explicit interface (sg_dma_is_restricted) for users to determine whether this is a restricted buffer. 3) some words improve, like using "rheap". Rebase on v6.9-rc7. v4: https://lore.kernel.org/linux-mediatek/20240112092014.23999-1-yong.wu@mediatek.com/ 1) Rename the heap name from "secure" to "restricted". suggested from Simon/Pekka. There are still several "secure" string in MTK file since we use ARM platform in which we call this "secure world"/ "secure command". v3: https://lore.kernel.org/linux-mediatek/20231212024607.3681-1-yong.wu@mediatek.com/ 1) Separate the secure heap to a common file(secure_heap.c) and mtk special file (secure_heap_mtk.c), and put all the tee related code into our special file. 2) About dt-binding, Add "mediatek," prefix since this is Mediatek TEE firmware definition. 3) Remove the normal CMA heap which is a draft for qcom. Rebase on v6.7-rc1. v2: https://lore.kernel.org/linux-mediatek/20231111111559.8218-1-yong.wu@mediatek.com/ 1) Move John's patches into the vcodec patchset since they use the new dma heap interface directly. https://lore.kernel.org/linux-mediatek/20231106120423.23364-1-yunfei.dong@mediatek.com/ 2) Reword the dt-binding description. 3) Rename the heap name from mtk_svp to secure_mtk_cm. This means the current vcodec/DRM upstream code doesn't match this. 4) Add a normal CMA heap. currently it should be a draft version. 5) Regarding the UUID, I still use hard code, but put it in a private data which allow the others could set their own UUID. What's more, UUID is necessary for the session with TEE. If we don't have it, we can't communicate with the TEE, including the get_uuid interface, which tries to make uuid more generic, not working. If there is other way to make UUID more general, please free to tell me. v1: https://lore.kernel.org/linux-mediatek/20230911023038.30649-1-yong.wu@mediatek.com/ Base on v6.6-rc1. Yong Wu (9): dt-bindings: reserved-memory: Add mediatek,dynamic-restricted-region scatterlist: Add a flag for the restricted memory lib/scatterlist: Add sg_dup_table dma-buf: heaps: Initialize a restricted heap dma-buf: heaps: restricted_heap: Add private heap ops dma-buf: heaps: restricted_heap: Add dma_ops dma-buf: heaps: restricted_heap: Add MediaTek restricted heap and heap_init dma-buf: heaps: restricted_heap_mtk: Add TEE memory service call dma_buf: heaps: restricted_heap_mtk: Add a new CMA heap .../mediatek,dynamic-restricted-region.yaml | 43 ++ drivers/dma-buf/heaps/Kconfig | 16 + drivers/dma-buf/heaps/Makefile | 4 +- drivers/dma-buf/heaps/restricted_heap.c | 219 +++++++++ drivers/dma-buf/heaps/restricted_heap.h | 45 ++ drivers/dma-buf/heaps/restricted_heap_mtk.c | 423 ++++++++++++++++++ drivers/dma-buf/heaps/system_heap.c | 27 +- include/linux/scatterlist.h | 36 ++ lib/scatterlist.c | 26 ++ 9 files changed, 812 insertions(+), 27 deletions(-) create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,dynamic-restricted-region.yaml create mode 100644 drivers/dma-buf/heaps/restricted_heap.c create mode 100644 drivers/dma-buf/heaps/restricted_heap.h create mode 100644 drivers/dma-buf/heaps/restricted_heap_mtk.c