| Message ID | 20240205182810.58382-11-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On Mon, Feb 05, 2024 at 07:28:08PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add a new Kconfig option for selecting the SHM Bridge mode of operation The Kconfig option is the least significant bit of this patch. I guess there isn't really a problem to describe, but let's at least mention that we're switching TZ to shmbridge mode and all. > for the TrustZone memory allocator. > > If enabled at build-time, it will still be checked for availability at > run-time. If the architecture doesn't support SHM Bridge, the allocator > will work just like in the default mode. "default mode"...SHMBRIDGE is the default mode from this point onwards. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s > Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride > Reviewed-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/firmware/qcom/Kconfig | 10 +++++ > drivers/firmware/qcom/qcom_tzmem.c | 65 +++++++++++++++++++++++++++++- > 2 files changed, 74 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig > index d24d83223867..af6f895c5adf 100644 > --- a/drivers/firmware/qcom/Kconfig > +++ b/drivers/firmware/qcom/Kconfig > @@ -28,6 +28,16 @@ config QCOM_TZMEM_MODE_DEFAULT > Use the default allocator mode. The memory is page-aligned, non-cachable > and contiguous. > > +config QCOM_TZMEM_MODE_SHMBRIDGE > + bool "SHM Bridge" > + help > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > + buffer. > + > + With this selected, all buffers passed to the TrustZone must be allocated > + using the TZMem allocator or else the TrustZone will refuse to use them. It's funny how this is the only place in the whole series I can find this mentioned. One could from this statement guess that the eluding scminvoke requires shmbridge and that this patch series exists solely to facilitate the requirement stated in this paragraph. Either this guess is correct and this should have been made clear in the commit messages, or I'm guessing wrong here, in which case I need some help to figure out why this series exists. Regards, Bjorn > + > endchoice > > config QCOM_SCM_DOWNLOAD_MODE_DEFAULT > diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c > index 44a062f2abd4..1ca3773263e5 100644 > --- a/drivers/firmware/qcom/qcom_tzmem.c > +++ b/drivers/firmware/qcom/qcom_tzmem.c > @@ -55,7 +55,70 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > > } > > -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ > +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) > + > +#include <linux/firmware/qcom/qcom_scm.h> > + > +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 > + > +static bool qcom_tzmem_using_shm_bridge; > + > +static int qcom_tzmem_init(void) > +{ > + int ret; > + > + ret = qcom_scm_shm_bridge_enable(); > + if (ret == -EOPNOTSUPP) { > + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); > + return 0; > + } > + > + if (!ret) > + qcom_tzmem_using_shm_bridge = true; > + > + return ret; > +} > + > +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) > +{ > + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms; > + int ret; > + > + if (!qcom_tzmem_using_shm_bridge) > + return 0; > + > + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); > + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; > + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; > + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); > + > + u64 *handle __free(kfree) = kzalloc(sizeof(*handle), GFP_KERNEL); > + if (!handle) > + return -ENOMEM; > + > + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, > + ipfn_and_s_perm, size_and_flags, > + QCOM_SCM_VMID_HLOS, handle); > + if (ret) > + return ret; > + > + pool->priv = no_free_ptr(handle); > + > + return 0; > +} > + > +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) > +{ > + u64 *handle = pool->priv; > + > + if (!qcom_tzmem_using_shm_bridge) > + return; > + > + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); > + kfree(handle); > +} > + > +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ > > /** > * qcom_tzmem_pool_new() - Create a new TZ memory pool. > -- > 2.40.1 >
On Sun, Feb 18, 2024 at 4:41 AM Bjorn Andersson <andersson@kernel.org> wrote: > > On Mon, Feb 05, 2024 at 07:28:08PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add a new Kconfig option for selecting the SHM Bridge mode of operation > > The Kconfig option is the least significant bit of this patch. I guess > there isn't really a problem to describe, but let's at least mention > that we're switching TZ to shmbridge mode and all. > > > for the TrustZone memory allocator. > > > > If enabled at build-time, it will still be checked for availability at > > run-time. If the architecture doesn't support SHM Bridge, the allocator > > will work just like in the default mode. > > "default mode"...SHMBRIDGE is the default mode from this point onwards. > Well, it is starting with patch 12/12 and only with arm64 defconfig. The Kconfig *default* is still the regular non-SHM bridge pool. I'll work on the naming convention. Bart [snip]
On Sun, Feb 18, 2024 at 4:41 AM Bjorn Andersson <andersson@kernel.org> wrote: > > On Mon, Feb 05, 2024 at 07:28:08PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > [snip] > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE > > + bool "SHM Bridge" > > + help > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > > + buffer. > > + > > + With this selected, all buffers passed to the TrustZone must be allocated > > + using the TZMem allocator or else the TrustZone will refuse to use them. > > It's funny how this is the only place in the whole series I can find > this mentioned. One could from this statement guess that the eluding > scminvoke requires shmbridge and that this patch series exists solely > to facilitate the requirement stated in this paragraph. > Yes, scminvoke *requires* SHM bridge. So does the wrapped key support. No, this is not the only reason as - as stated by Srini - it improves overall safety of the system for all users. > Either this guess is correct and this should have been made clear in > the commit messages, or I'm guessing wrong here, in which case I need > some help to figure out why this series exists. > This series exists and IMO should get upstream soon to facilitate adding new security features (in addition to improving existing ones). As there are at least two such features in development (mentioned above) pushing this series upstream will make it easier to develop them independently. Bart [snip]
On Thu, Feb 22, 2024 at 05:24:19PM +0100, Bartosz Golaszewski wrote: > On Sun, Feb 18, 2024 at 4:41 AM Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Mon, Feb 05, 2024 at 07:28:08PM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > [snip] > > > > > > > +config QCOM_TZMEM_MODE_SHMBRIDGE > > > + bool "SHM Bridge" > > > + help > > > + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as > > > + in the 'Default' allocator but is also explicitly marked as an SHM Bridge > > > + buffer. > > > + > > > + With this selected, all buffers passed to the TrustZone must be allocated > > > + using the TZMem allocator or else the TrustZone will refuse to use them. > > > > It's funny how this is the only place in the whole series I can find > > this mentioned. One could from this statement guess that the eluding > > scminvoke requires shmbridge and that this patch series exists solely > > to facilitate the requirement stated in this paragraph. > > > > Yes, scminvoke *requires* SHM bridge. So does the wrapped key support. > No, this is not the only reason as - as stated by Srini - it improves > overall safety of the system for all users. > > > Either this guess is correct and this should have been made clear in > > the commit messages, or I'm guessing wrong here, in which case I need > > some help to figure out why this series exists. > > > > This series exists and IMO should get upstream soon to facilitate > adding new security features (in addition to improving existing ones). This needs to be stated in the cover letter/commit messages. > As there are at least two such features in development (mentioned > above) pushing this series upstream will make it easier to develop > them independently. > Show me the work-in-progress code and I will entertain this argument. Regards, Bjorn > Bart > > [snip]
diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig index d24d83223867..af6f895c5adf 100644 --- a/drivers/firmware/qcom/Kconfig +++ b/drivers/firmware/qcom/Kconfig @@ -28,6 +28,16 @@ config QCOM_TZMEM_MODE_DEFAULT Use the default allocator mode. The memory is page-aligned, non-cachable and contiguous. +config QCOM_TZMEM_MODE_SHMBRIDGE + bool "SHM Bridge" + help + Use Qualcomm Shared Memory Bridge. The memory has the same alignment as + in the 'Default' allocator but is also explicitly marked as an SHM Bridge + buffer. + + With this selected, all buffers passed to the TrustZone must be allocated + using the TZMem allocator or else the TrustZone will refuse to use them. + endchoice config QCOM_SCM_DOWNLOAD_MODE_DEFAULT diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c index 44a062f2abd4..1ca3773263e5 100644 --- a/drivers/firmware/qcom/qcom_tzmem.c +++ b/drivers/firmware/qcom/qcom_tzmem.c @@ -55,7 +55,70 @@ static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) } -#endif /* CONFIG_QCOM_TZMEM_MODE_DEFAULT */ +#elif IS_ENABLED(CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE) + +#include <linux/firmware/qcom/qcom_scm.h> + +#define QCOM_SHM_BRIDGE_NUM_VM_SHIFT 9 + +static bool qcom_tzmem_using_shm_bridge; + +static int qcom_tzmem_init(void) +{ + int ret; + + ret = qcom_scm_shm_bridge_enable(); + if (ret == -EOPNOTSUPP) { + dev_info(qcom_tzmem_dev, "SHM Bridge not supported\n"); + return 0; + } + + if (!ret) + qcom_tzmem_using_shm_bridge = true; + + return ret; +} + +static int qcom_tzmem_init_pool(struct qcom_tzmem_pool *pool) +{ + u64 pfn_and_ns_perm, ipfn_and_s_perm, size_and_flags, ns_perms; + int ret; + + if (!qcom_tzmem_using_shm_bridge) + return 0; + + ns_perms = (QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ); + pfn_and_ns_perm = (u64)pool->pbase | ns_perms; + ipfn_and_s_perm = (u64)pool->pbase | ns_perms; + size_and_flags = pool->size | (1 << QCOM_SHM_BRIDGE_NUM_VM_SHIFT); + + u64 *handle __free(kfree) = kzalloc(sizeof(*handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; + + ret = qcom_scm_shm_bridge_create(qcom_tzmem_dev, pfn_and_ns_perm, + ipfn_and_s_perm, size_and_flags, + QCOM_SCM_VMID_HLOS, handle); + if (ret) + return ret; + + pool->priv = no_free_ptr(handle); + + return 0; +} + +static void qcom_tzmem_cleanup_pool(struct qcom_tzmem_pool *pool) +{ + u64 *handle = pool->priv; + + if (!qcom_tzmem_using_shm_bridge) + return; + + qcom_scm_shm_bridge_delete(qcom_tzmem_dev, *handle); + kfree(handle); +} + +#endif /* CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE */ /** * qcom_tzmem_pool_new() - Create a new TZ memory pool.