| Message ID | 20240515112308.10171-3-yong.wu@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | dma-buf: heaps: Add restricted heap | expand |
Am 15.05.24 um 13:23 schrieb Yong Wu: > Introduce a FLAG for the restricted memory which means the memory is > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > Currently we don't use sg_dma_unmark_restricted, thus this interface > has not been added. Why should that be part of the scatterlist? It doesn't seem to affect any of it's functionality. As far as I can see the scatterlist shouldn't be the transport of this kind of information. Regards, Christian. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > include/linux/scatterlist.h | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 77df3d7b18a6..a6ad9018eca0 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct scatterlist *sg) > > #define SG_DMA_BUS_ADDRESS (1 << 0) > #define SG_DMA_SWIOTLB (1 << 1) > +#define SG_DMA_RESTRICTED (2 << 1) > > /** > * sg_dma_is_bus_address - Return whether a given segment was marked > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > sg->dma_flags |= SG_DMA_SWIOTLB; > } > > +/** > + * sg_dma_mark_restricted - Mark the scatterlist for restricted buffer. > + * @sg: SG entry > + * > + * Description: > + * Marks a a scatterlist for the restricted buffer that may be inaccessiable > + * in kernel if it is protected. > + */ > +static inline void sg_dma_mark_restricted(struct scatterlist *sg) > +{ > + sg->dma_flags |= SG_DMA_RESTRICTED; > +} > + > +/** > + * sg_dma_is_restricted - Return whether the scatterlist was marked as restricted > + * buffer. > + * @sg: SG entry > + * > + * Description: > + * Returns true if the scatterlist was marked as restricted buffer. > + */ > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return sg->dma_flags & SG_DMA_RESTRICTED; > +} > #else > > static inline bool sg_dma_is_bus_address(struct scatterlist *sg) > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > { > } > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return false; > +} > + > +static inline void sg_dma_mark_restrited(struct scatterlist *sg) > +{ > +} > #endif /* CONFIG_NEED_SG_DMA_FLAGS */ > > /**
Il 15/05/24 13:23, Yong Wu ha scritto: > Introduce a FLAG for the restricted memory which means the memory is > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > Currently we don't use sg_dma_unmark_restricted, thus this interface > has not been added. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > include/linux/scatterlist.h | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 77df3d7b18a6..a6ad9018eca0 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct scatterlist *sg) > > #define SG_DMA_BUS_ADDRESS (1 << 0) > #define SG_DMA_SWIOTLB (1 << 1) > +#define SG_DMA_RESTRICTED (2 << 1) I think you wanted to write (1 << 2) here :-) Cheers, Angelo > > /** > * sg_dma_is_bus_address - Return whether a given segment was marked > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > sg->dma_flags |= SG_DMA_SWIOTLB; > } > > +/** > + * sg_dma_mark_restricted - Mark the scatterlist for restricted buffer. > + * @sg: SG entry > + * > + * Description: > + * Marks a a scatterlist for the restricted buffer that may be inaccessiable > + * in kernel if it is protected. > + */ > +static inline void sg_dma_mark_restricted(struct scatterlist *sg) > +{ > + sg->dma_flags |= SG_DMA_RESTRICTED; > +} > + > +/** > + * sg_dma_is_restricted - Return whether the scatterlist was marked as restricted > + * buffer. > + * @sg: SG entry > + * > + * Description: > + * Returns true if the scatterlist was marked as restricted buffer. > + */ > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return sg->dma_flags & SG_DMA_RESTRICTED; > +} > #else > > static inline bool sg_dma_is_bus_address(struct scatterlist *sg) > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct scatterlist *sg) > { > } > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > +{ > + return false; > +} > + > +static inline void sg_dma_mark_restrited(struct scatterlist *sg) > +{ > +} > #endif /* CONFIG_NEED_SG_DMA_FLAGS */ > > /**
On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Am 15.05.24 um 13:23 schrieb Yong Wu: > > Introduce a FLAG for the restricted memory which means the memory > is > > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > > > Currently we don't use sg_dma_unmark_restricted, thus this > interface > > has not been added. > > Why should that be part of the scatterlist? It doesn't seem to > affect > any of it's functionality. > > As far as I can see the scatterlist shouldn't be the transport of > this > kind of information. Thanks for the review. I will remove this. In our user scenario, DRM will import these buffers and check if this is a restricted buffer. If yes, it will use secure GCE takes over. If this judgment is not suitable to be placed in scatterlist. I don't know if it is ok to limit this inside dma-buf. Adding such an interface: static bool dma_buf_is_restricted(struct dma_buf *dmabuf) { return !strncmp(dmabuf->exp_name, "restricted", 10); } Thanks. > > Regards, > Christian. > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > include/linux/scatterlist.h | 34 > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/include/linux/scatterlist.h > b/include/linux/scatterlist.h > > index 77df3d7b18a6..a6ad9018eca0 100644 > > --- a/include/linux/scatterlist.h > > +++ b/include/linux/scatterlist.h > > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct > scatterlist *sg) > > > > #define SG_DMA_BUS_ADDRESS(1 << 0) > > #define SG_DMA_SWIOTLB(1 << 1) > > +#define SG_DMA_RESTRICTED(2 << 1) > > > > /** > > * sg_dma_is_bus_address - Return whether a given segment was > marked > > @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct > scatterlist *sg) > > sg->dma_flags |= SG_DMA_SWIOTLB; > > } > > > > +/** > > + * sg_dma_mark_restricted - Mark the scatterlist for restricted > buffer. > > + * @sg:SG entry > > + * > > + * Description: > > + * Marks a a scatterlist for the restricted buffer that may be > inaccessiable > > + * in kernel if it is protected. > > + */ > > +static inline void sg_dma_mark_restricted(struct scatterlist *sg) > > +{ > > +sg->dma_flags |= SG_DMA_RESTRICTED; > > +} > > + > > +/** > > + * sg_dma_is_restricted - Return whether the scatterlist was > marked as restricted > > + * buffer. > > + * @sg:SG entry > > + * > > + * Description: > > + * Returns true if the scatterlist was marked as restricted > buffer. > > + */ > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > > +{ > > +return sg->dma_flags & SG_DMA_RESTRICTED; > > +} > > #else > > > > static inline bool sg_dma_is_bus_address(struct scatterlist *sg) > > @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct > scatterlist *sg) > > { > > } > > > > +static inline bool sg_dma_is_restricted(struct scatterlist *sg) > > +{ > > +return false; > > +} > > + > > +static inline void sg_dma_mark_restrited(struct scatterlist *sg) > > +{ > > +} > > #endif/* CONFIG_NEED_SG_DMA_FLAGS */ > > > > /** > >
On Thu, 2024-05-16 at 11:59 +0200, AngeloGioacchino Del Regno wrote: > Il 15/05/24 13:23, Yong Wu ha scritto: > > Introduce a FLAG for the restricted memory which means the memory > > is > > protected by TEE or hypervisor, then it's inaccessiable for kernel. > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > interface > > has not been added. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > --- > > include/linux/scatterlist.h | 34 > > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/include/linux/scatterlist.h > > b/include/linux/scatterlist.h > > index 77df3d7b18a6..a6ad9018eca0 100644 > > --- a/include/linux/scatterlist.h > > +++ b/include/linux/scatterlist.h > > @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct > > scatterlist *sg) > > > > #define SG_DMA_BUS_ADDRESS (1 << 0) > > #define SG_DMA_SWIOTLB (1 << 1) > > +#define SG_DMA_RESTRICTED (2 << 1) > > I think you wanted to write (1 << 2) here :-) Apparently, you are right:) Thanks. > > Cheers, > Angelo
Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> Am 15.05.24 um 13:23 schrieb Yong Wu: >>> Introduce a FLAG for the restricted memory which means the memory >> is >>> protected by TEE or hypervisor, then it's inaccessiable for kernel. >>> >>> Currently we don't use sg_dma_unmark_restricted, thus this >> interface >>> has not been added. >> Why should that be part of the scatterlist? It doesn't seem to >> affect >> any of it's functionality. >> >> As far as I can see the scatterlist shouldn't be the transport of >> this >> kind of information. > Thanks for the review. I will remove this. > > In our user scenario, DRM will import these buffers and check if this > is a restricted buffer. If yes, it will use secure GCE takes over. > > If this judgment is not suitable to be placed in scatterlist. I don't > know if it is ok to limit this inside dma-buf. Adding such an > interface: > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > { > return !strncmp(dmabuf->exp_name, "restricted", 10); > } No, usually stuff like that doesn't belong into DMA buf either. Question here really is who controls the security status of the memory backing the buffer? In other words who tells the exporter that it should allocate and fill a buffer with encrypted data? If that is userspace then that is part of the format information and it is also userspace who should tell the importer that it needs to work with encrypted data. The kernel is intentionally not involved in stuff like that. Regards, Christian. > > Thanks. > >> Regards, >> Christian. >> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com> >>> --- >>> include/linux/scatterlist.h | 34 >> ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/include/linux/scatterlist.h >> b/include/linux/scatterlist.h >>> index 77df3d7b18a6..a6ad9018eca0 100644 >>> --- a/include/linux/scatterlist.h >>> +++ b/include/linux/scatterlist.h >>> @@ -282,6 +282,7 @@ static inline void sg_unmark_end(struct >> scatterlist *sg) >>> >>> #define SG_DMA_BUS_ADDRESS(1 << 0) >>> #define SG_DMA_SWIOTLB(1 << 1) >>> +#define SG_DMA_RESTRICTED(2 << 1) >>> >>> /** >>> * sg_dma_is_bus_address - Return whether a given segment was >> marked >>> @@ -352,6 +353,31 @@ static inline void sg_dma_mark_swiotlb(struct >> scatterlist *sg) >>> sg->dma_flags |= SG_DMA_SWIOTLB; >>> } >>> >>> +/** >>> + * sg_dma_mark_restricted - Mark the scatterlist for restricted >> buffer. >>> + * @sg:SG entry >>> + * >>> + * Description: >>> + * Marks a a scatterlist for the restricted buffer that may be >> inaccessiable >>> + * in kernel if it is protected. >>> + */ >>> +static inline void sg_dma_mark_restricted(struct scatterlist *sg) >>> +{ >>> +sg->dma_flags |= SG_DMA_RESTRICTED; >>> +} >>> + >>> +/** >>> + * sg_dma_is_restricted - Return whether the scatterlist was >> marked as restricted >>> + * buffer. >>> + * @sg:SG entry >>> + * >>> + * Description: >>> + * Returns true if the scatterlist was marked as restricted >> buffer. >>> + */ >>> +static inline bool sg_dma_is_restricted(struct scatterlist *sg) >>> +{ >>> +return sg->dma_flags & SG_DMA_RESTRICTED; >>> +} >>> #else >>> >>> static inline bool sg_dma_is_bus_address(struct scatterlist *sg) >>> @@ -372,6 +398,14 @@ static inline void sg_dma_mark_swiotlb(struct >> scatterlist *sg) >>> { >>> } >>> >>> +static inline bool sg_dma_is_restricted(struct scatterlist *sg) >>> +{ >>> +return false; >>> +} >>> + >>> +static inline void sg_dma_mark_restrited(struct scatterlist *sg) >>> +{ >>> +} >>> #endif/* CONFIG_NEED_SG_DMA_FLAGS */ >>> >>> /** >>
Hi Christian, On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote: > Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > > > > > External email : Please do not click links or open attachments > > > until > > > you have verified the sender or the content. > > > Am 15.05.24 um 13:23 schrieb Yong Wu: > > > > Introduce a FLAG for the restricted memory which means the > > > > memory > > > > > > is > > > > protected by TEE or hypervisor, then it's inaccessiable for > > > > kernel. > > > > > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > > > > > interface > > > > has not been added. > > > > > > Why should that be part of the scatterlist? It doesn't seem to > > > affect > > > any of it's functionality. > > > > > > As far as I can see the scatterlist shouldn't be the transport of > > > this > > > kind of information. > > > > Thanks for the review. I will remove this. > > > > In our user scenario, DRM will import these buffers and check if > > this > > is a restricted buffer. If yes, it will use secure GCE takes over. > > > > If this judgment is not suitable to be placed in scatterlist. I > > don't > > know if it is ok to limit this inside dma-buf. Adding such an > > interface: > > > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > > { > > return !strncmp(dmabuf->exp_name, "restricted", 10); > > } > > No, usually stuff like that doesn't belong into DMA buf either. > > Question here really is who controls the security status of the > memory > backing the buffer? > > In other words who tells the exporter that it should allocate and > fill a > buffer with encrypted data? > > If that is userspace then that is part of the format information and > it > is also userspace who should tell the importer that it needs to work > with encrypted data. > > The kernel is intentionally not involved in stuff like that. > Here is the expected protected content buffer flow in DRM: 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by DMA_HEAP_IOCTL_ALLOC. 2) userspace imports that dma-buf into the device using prime for the drm_file. 3) userspace uses the already implemented driver import code for the special cases of protected content buffer. In the step 3), we need to verify the dma-buf is allocated from "restricted_mtk_cma", but there is no way to pass the secure flag or private data from userspace to the import interface in DRM driver. So I can only verify it like this now: struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, struct sg_table *sg) { struct mtk_gem_obj *mtk_gem; /* check if the entries in the sg_table are contiguous */ if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { DRM_ERROR("sg_table is not contiguous"); return ERR_PTR(-EINVAL); } mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); if (IS_ERR(mtk_gem)) return ERR_CAST(mtk_gem); + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); mtk_gem->dma_addr = sg_dma_address(sg->sgl); mtk_gem->size = attach->dmabuf->size; mtk_gem->sg = sg; return &mtk_gem->base; } I think I have the same problem as the ECC_FLAG mention in: https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ I think it would be better to have the user configurable private information in dma-buf, so all the drivers who have the same requirement can get their private information from dma-buf directly and no need to change or add the interface. What's your opinion in this point? Regards, Jason-JH.Lin > Regards, > Christian.
> > > > Here is the expected protected content buffer flow in DRM: > > 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" > > by > > DMA_HEAP_IOCTL_ALLOC. > > 2) userspace imports that dma-buf into the device using prime for > > the > > drm_file. > > 3) userspace uses the already implemented driver import code for > > the > > special cases of protected content buffer. > > What is so special on that case? The special case simply means the the protected content buffer. > > > In the step 3), we need to verify the dma-buf is allocated from > > "restricted_mtk_cma", but there is no way to pass the secure flag > > or > > private data from userspace to the import interface in DRM driver. > > Why do you need to verify that? I need to know the imported buffer is allocated from restricted cma and mark it as a secure buffer in mediatek-drm driver. Then, I will add some configuration to the hardware if the buffer is secure buffer, so that it can get the permission to access the secure buffer. > > > So I can only verify it like this now: > > struct drm_gem_object *mtk_gem_prime_import_sg_table(struct > > drm_device > > *dev, struct dma_buf_attachment *attach, struct sg_table *sg) > > { > > struct mtk_gem_obj *mtk_gem; > > > > /* check if the entries in the sg_table are contiguous */ > > if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { > > DRM_ERROR("sg_table is not contiguous"); > > return ERR_PTR(-EINVAL); > > } > > mtk_gem = mtk_gem_init(dev, attach->dmabuf->size); > > if (IS_ERR(mtk_gem)) > > return ERR_CAST(mtk_gem); > > > > + mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, > > "restricted", > > 10)); > > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > > mtk_gem->size = attach->dmabuf->size; > > mtk_gem->sg = sg; > > > > return &mtk_gem->base; > > } > > Complete NAK from my side to that approach. Importing of a DMA-buf > should be independent of the exporter. > > What you could do is to provide the secure buffer from a device and > not a device heap. > You mean I should allocate buffer in mediate-drm driver not userspace? I just have modified this to userspace by the comment here: https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.369-3-shawn.sung@mediatek.com/#25806766 > > I think I have the same problem as the ECC_FLAG mention in: > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > I think it would be better to have the user configurable private > > information in dma-buf, so all the drivers who have the same > > requirement can get their private information from dma-buf directly > > and > > no need to change or add the interface. > > > > What's your opinion in this point? > > Well of hand I don't see the need for that. > > What happens if you get a non-secure buffer imported in your secure > device? We use the same mediatek-drm driver for secure and non-secure buffer. If non-secure buffer imported to mediatek-drm driver, it's go to the normal flow with normal hardware settings. We use different configurations to make hardware have different permission to access the buffer it should access. So if we can't get the information of "the buffer is allocated from restricted_mtk_cma" when importing the buffer into the driver, we won't be able to configure the hardware correctly. Regards, Jason-JH.Lin > > Regards, > Christian. > > > Regards, > > Jason-JH.Lin > > > > > Regards, > > > Christian. > > > > ************* MEDIATEK Confidentiality Notice > > ******************** > > The information contained in this e-mail message (including any > > attachments) may be confidential, proprietary, privileged, or > > otherwise > > exempt from disclosure under applicable laws. It is intended to be > > conveyed only to the designated recipient(s). Any use, > > dissemination, > > distribution, printing, retaining or copying of this e-mail > > (including its > > attachments) by unintended recipient(s) is strictly prohibited and > > may > > be unlawful. If you are not an intended recipient of this e-mail, > > or believe > > > > that you have received this e-mail in error, please notify the > > sender > > immediately (by replying to this e-mail), delete any and all copies > > of > > this e-mail (including any attachments) from your system, and do > > not > > disclose the content of this e-mail to any other person. Thank you! >
On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > I think it would be better to have the user configurable > private > > > > > information in dma-buf, so all the drivers who have the same > > > > > requirement can get their private information from dma-buf > directly > > > > > and > > > > > no need to change or add the interface. > > > > > > > What's your opinion in this point? > > > > > Well of hand I don't see the need for that. > > > > > What happens if you get a non-secure buffer imported in your > secure > > > > device? > > > > > > We use the same mediatek-drm driver for secure and non-secure > buffer. > > > If non-secure buffer imported to mediatek-drm driver, it's go to > the > > > normal flow with normal hardware settings. > > > > > > We use different configurations to make hardware have different > > > permission to access the buffer it should access. > > > > > > So if we can't get the information of "the buffer is allocated > from > > > restricted_mtk_cma" when importing the buffer into the driver, we > won't > > > be able to configure the hardware correctly. > > > > Why can't you get this information from userspace? > > Same reason amd and i915/xe also pass this around internally in the > kernel, it's just that for those gpus the render and kms node are the > same > driver so this is easy. > > But on arm you have split designs everywhere and dma-buf > import/export, so > something else is needed. And neither current kms uapi nor > protocols/extensions have provisions for this (afaik) because it > works on > the big gpus, and on android it's just hacked up with backchannels. > > So yeah essentially I think we probably need something like this, as > much > as it sucks. I see it somewhat similar to handling pcip2pdma > limitations > in the kernel too. > > Not sure where/how it should be handled though, and maybe I've missed > something around protocols, in which case I guess we should add some > secure buffer flags to the ADDFB2 ioctl. Thanks for your hint, I'll try to add the secure flag to the ADDFB2 ioctl. If it works, I'll send the patch. Regards, Jason-JH.Lin > -Sima
Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > I think it would be better to have the user configurable > > private > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > requirement can get their private information from dma-buf > > directly > > > > > > and > > > > > > no need to change or add the interface. > > > > > > > > What's your opinion in this point? > > > > > > Well of hand I don't see the need for that. > > > > > > What happens if you get a non-secure buffer imported in your > > secure > > > > > device? > > > > > > > > We use the same mediatek-drm driver for secure and non-secure > > buffer. > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > the > > > > normal flow with normal hardware settings. > > > > > > > > We use different configurations to make hardware have different > > > > permission to access the buffer it should access. > > > > > > > > So if we can't get the information of "the buffer is allocated > > from > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > won't > > > > be able to configure the hardware correctly. > > > > > > Why can't you get this information from userspace? > > > > Same reason amd and i915/xe also pass this around internally in the > > kernel, it's just that for those gpus the render and kms node are the > > same > > driver so this is easy. > > The reason I ask is that encryption here looks just like another parameter for the buffer, e.g. like format, stride, tilling etc.. So instead of this during buffer import: mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); mtk_gem->dma_addr = sg_dma_address(sg->sgl); mtk_gem->size = attach->dmabuf->size; mtk_gem->sg = sg; You can trivially say during use hey this buffer is encrypted. At least that's my 10 mile high view, maybe I'm missing some extensive key exchange or something like that. > > > But on arm you have split designs everywhere and dma-buf > > import/export, so > > something else is needed. And neither current kms uapi nor > > protocols/extensions have provisions for this (afaik) because it > > works on > > the big gpus, and on android it's just hacked up with backchannels. > > > > So yeah essentially I think we probably need something like this, as > > much > > as it sucks. I see it somewhat similar to handling pcip2pdma > > limitations > > in the kernel too. > > > > Not sure where/how it should be handled though, and maybe I've missed > > something around protocols, in which case I guess we should add some > > secure buffer flags to the ADDFB2 ioctl. > > Thanks for your hint, I'll try to add the secure flag to the ADDFB2 > ioctl. If it works, I'll send the patch. Yeah, exactly what I would suggest as well. I'm not an expert for that part, but as far as I know we already have bunch of device specific tilling flags in there. Adding an MTK_ENCRYPTED flag should be trivial. Regards, Christian. > > Regards, > Jason-JH.Lin > > > -Sima > > ************* MEDIATEK Confidentiality Notice ******************** > The information contained in this e-mail message (including any > attachments) may be confidential, proprietary, privileged, or otherwise > exempt from disclosure under applicable laws. It is intended to be > conveyed only to the designated recipient(s). Any use, dissemination, > distribution, printing, retaining or copying of this e-mail (including its > attachments) by unintended recipient(s) is strictly prohibited and may > be unlawful. If you are not an intended recipient of this e-mail, or believe > that you have received this e-mail in error, please notify the sender > immediately (by replying to this e-mail), delete any and all copies of > this e-mail (including any attachments) from your system, and do not > disclose the content of this e-mail to any other person. Thank you!
On Thu, Jun 27, 2024 at 08:57:40AM GMT, Christian König wrote: > Am 27.06.24 um 05:21 schrieb Jason-JH Lin (林睿祥): > > > > On Wed, 2024-06-26 at 19:56 +0200, Daniel Vetter wrote: > > > > External email : Please do not click links or open attachments > > until > > > you have verified the sender or the content. > > > On Wed, Jun 26, 2024 at 12:49:02PM +0200, Christian König wrote: > > > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/ > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > > > information in dma-buf, so all the drivers who have the same > > > > > > > requirement can get their private information from dma-buf > > > directly > > > > > > > and > > > > > > > no need to change or add the interface. > > > > > > > > > What's your opinion in this point? > > > > > > > Well of hand I don't see the need for that. > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > > > device? > > > > > > > > We use the same mediatek-drm driver for secure and > > non-secure > > > buffer. > > > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > > > normal flow with normal hardware settings. > > > > > > > > We use different configurations to make hardware have > > different > > > > > permission to access the buffer it should access. > > > > > > > > So if we can't get the information of "the buffer is > > allocated > > > from > > > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > > > be able to configure the hardware correctly. > > > > > > Why can't you get this information from userspace? > > > > Same reason amd and i915/xe also pass this around internally in the > > > kernel, it's just that for those gpus the render and kms node are the > > > same > > > driver so this is easy. > > > > > The reason I ask is that encryption here looks just like another parameter > for the buffer, e.g. like format, stride, tilling etc.. > > So instead of this during buffer import: > > mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted", 10)); > mtk_gem->dma_addr = sg_dma_address(sg->sgl); > mtk_gem->size = attach->dmabuf->size; > mtk_gem->sg = sg; > > You can trivially say during use hey this buffer is encrypted. > > At least that's my 10 mile high view, maybe I'm missing some extensive key > exchange or something like that. That doesn't work in all cases, unfortunately. If you're doing secure video playback, the firmware is typically in charge of the frame decryption/decoding, and you'd get dma-buf back that aren't accessible by the CPU (or at least, not at the execution level Linux runs with). So nobody can map that buffer, and the firmware driver is the one who knows that this buffer cannot be accessed by anyone. Putting this on the userspace to know would be pretty weird, and wouldn't solve the case where the kernel would try to map it. Maxime
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(+)