| Message ID | 20240515112308.10171-3-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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.
Am 25.06.24 um 13:02 schrieb Jason-JH Lin (林睿祥): > > 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. What is so special on that case? > > 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? > > 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. > 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? 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!
> > > > 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! >
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. 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. > > > > > 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. > 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? Regards, Christian. > > 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! > > > > ************* 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:17 schrieb Jason-JH Lin (林睿祥): > > 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. Well stuff like that is usually device driver specific. So you should probably use something device specific which tells the driver that this buffer is encrypted. > > 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. Well do you really need a separate heap for that? In other words is only a certain part of the system memory capable of being encrypted? Or would the "normal" CMA heap do as well and you only need to setup your hardware properly for encryption? Additional to that in most other drivers buffer sharing and encryption are two separate things. In other words other drivers do something like this: 1. Allocate the buffer. 2. Import the buffer using DRM_IOCTL_PRIME_FD_TO_HANDLE. 3. Set additional buffer properties, e.g. format, stride, tilling, if it's secure, which encryption key to use, where to map etc... So as far as I can see the problem you are facing is that you try to mangle everything into DRM_IOCTL_PRIME_FD_TO_HANDLE. Why not make that a separate IOCTL? I mean we intentionally don't provide things like format, stride, tilling etc.. to DRM_IOCTL_PRIME_FD_TO_HANDLE. Encryption is just another of those parameters. Regards, Christian. > > > > 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 > > ************* 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!
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
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. > 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. Regards, Christian. > > 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
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. 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? Thanks, Christian. > 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.
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. >>> 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. 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 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 >
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 */ /**
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(+)