linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
       [not found] <20210716022332.GC3232@sequoia>
@ 2021-07-16  2:56 ` Vikas Gupta
       [not found] ` <CAHLZf_t5U1bh1H8sULbJz7xrZ-r3Dcmxuw9MMmG2fehS3C72uQ@mail.gmail.com>
  1 sibling, 0 replies; 7+ messages in thread
From: Vikas Gupta @ 2021-07-16  2:56 UTC (permalink / raw)
  To: Jens Wiklander, Allen Pais, Sumit Garg, Peter Huewe,
	Jarkko Sakkinen, Jason Gunthorpe, Vikas Gupta, Tyler Hicks
  Cc: Thirupathaiah Annapureddy, Pavel Tatashin,
	Rafał Miłecki, op-tee, linux-integrity,
	BCM Kernel Feedback, linux-mips, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 3407 bytes --]

Hi Tyler/Allen,
 The patch looks good to me.

Thanks,
Vikas


> > From: Allen Pais <apais@linux.microsoft.com>
> >
> > Implement a .shutdown hook that will be called during a kexec operation
> > so that the TEE shared memory, session, and context that were set up
> > during .probe can be properly freed/closed.
> >
> > Additionally, don't use dma-buf backed shared memory for the
> > fw_shm_pool. dma-buf backed shared memory cannot be reliably freed and
> > unregistered during a kexec operation even when tee_shm_free() is called
> > on the shm from a .shutdown hook. The problem occurs because
> > dma_buf_put() calls fput() which then uses task_work_add(), with the
> > TWA_RESUME parameter, to queue tee_shm_release() to be called before the
> > current task returns to user mode. However, the current task never
> > returns to user mode before the kexec completes so the memory is never
> > freed nor unregistered.
> >
> > Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory
> > allocation so that tee_shm_free() can directly call tee_shm_release().
> > This will ensure that the shm can be freed and unregistered during a
> > kexec operation.
> >
> > Fixes: 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> > Co-developed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> >  drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
> > index ed10da5313e8..a5bf4c3f6dc7 100644
> > --- a/drivers/firmware/broadcom/tee_bnxt_fw.c
> > +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
> > @@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
> >
> >       pvt_data.dev = dev;
> >
> > -     fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
> > -                                 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
> > +     fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
> >       if (IS_ERR(fw_shm_pool)) {
> > -             dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
> > +             dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
> >               err = PTR_ERR(fw_shm_pool);
> >               goto out_sess;
> >       }
> > @@ -242,6 +241,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
> >       return 0;
> >  }
> >
> > +static void tee_bnxt_fw_shutdown(struct device *dev)
> > +{
> > +     tee_shm_free(pvt_data.fw_shm_pool);
> > +     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
> > +     tee_client_close_context(pvt_data.ctx);
> > +     pvt_data.ctx = NULL;
> > +}
> > +
> >  static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
> >       {UUID_INIT(0x6272636D, 0x2019, 0x0716,
> >                   0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
> > @@ -257,6 +264,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = {
> >               .bus            = &tee_bus_type,
> >               .probe          = tee_bnxt_fw_probe,
> >               .remove         = tee_bnxt_fw_remove,
> > +             .shutdown       = tee_bnxt_fw_shutdown,
> >       },
> >  };
> >
> > --
> > 2.25.1
> >
>
> ----- End forwarded message -----

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
       [not found] ` <CAHLZf_t5U1bh1H8sULbJz7xrZ-r3Dcmxuw9MMmG2fehS3C72uQ@mail.gmail.com>
@ 2021-07-19 10:49   ` Jens Wiklander
  2021-07-20  2:32     ` Florian Fainelli
  0 siblings, 1 reply; 7+ messages in thread
From: Jens Wiklander @ 2021-07-19 10:49 UTC (permalink / raw)
  To: Vikas Gupta, Rafał Miłecki
  Cc: Tyler Hicks, Allen Pais, Sumit Garg, Peter Huewe,
	Jarkko Sakkinen, Jason Gunthorpe, Thirupathaiah Annapureddy,
	Pavel Tatashin, OP-TEE TrustedFirmware, linux-integrity,
	BCM Kernel Feedback, linux-mips, Linux Kernel Mailing List

Hi,

On Fri, Jul 16, 2021 at 4:48 AM Vikas Gupta <vikas.gupta@broadcom.com> wrote:
>
> Hi Allen/Tyler,
>  The patch looks good to me.

Thanks.

Rafal, is it OK if I include this patch together with the rest of the
patches in this patch set in a pull request to arm-soc?

Thanks,
Jens

>
> Thanks,
> Vikas
>
>>
>> > From: Allen Pais <apais@linux.microsoft.com>
>> >
>> > Implement a .shutdown hook that will be called during a kexec operation
>> > so that the TEE shared memory, session, and context that were set up
>> > during .probe can be properly freed/closed.
>> >
>> > Additionally, don't use dma-buf backed shared memory for the
>> > fw_shm_pool. dma-buf backed shared memory cannot be reliably freed and
>> > unregistered during a kexec operation even when tee_shm_free() is called
>> > on the shm from a .shutdown hook. The problem occurs because
>> > dma_buf_put() calls fput() which then uses task_work_add(), with the
>> > TWA_RESUME parameter, to queue tee_shm_release() to be called before the
>> > current task returns to user mode. However, the current task never
>> > returns to user mode before the kexec completes so the memory is never
>> > freed nor unregistered.
>> >
>> > Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory
>> > allocation so that tee_shm_free() can directly call tee_shm_release().
>> > This will ensure that the shm can be freed and unregistered during a
>> > kexec operation.
>> >
>> > Fixes: 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
>> > Co-developed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>> > ---
>> >  drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++---
>> >  1 file changed, 11 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
>> > index ed10da5313e8..a5bf4c3f6dc7 100644
>> > --- a/drivers/firmware/broadcom/tee_bnxt_fw.c
>> > +++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
>> > @@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
>> >
>> >       pvt_data.dev = dev;
>> >
>> > -     fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
>> > -                                 TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
>> > +     fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
>> >       if (IS_ERR(fw_shm_pool)) {
>> > -             dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
>> > +             dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
>> >               err = PTR_ERR(fw_shm_pool);
>> >               goto out_sess;
>> >       }
>> > @@ -242,6 +241,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
>> >       return 0;
>> >  }
>> >
>> > +static void tee_bnxt_fw_shutdown(struct device *dev)
>> > +{
>> > +     tee_shm_free(pvt_data.fw_shm_pool);
>> > +     tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
>> > +     tee_client_close_context(pvt_data.ctx);
>> > +     pvt_data.ctx = NULL;
>> > +}
>> > +
>> >  static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
>> >       {UUID_INIT(0x6272636D, 0x2019, 0x0716,
>> >                   0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
>> > @@ -257,6 +264,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = {
>> >               .bus            = &tee_bus_type,
>> >               .probe          = tee_bnxt_fw_probe,
>> >               .remove         = tee_bnxt_fw_remove,
>> > +             .shutdown       = tee_bnxt_fw_shutdown,
>> >       },
>> >  };
>> >
>> > --
>> > 2.25.1
>> >
>>
>> ----- End forwarded message -----

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
  2021-07-19 10:49   ` Jens Wiklander
@ 2021-07-20  2:32     ` Florian Fainelli
  2021-07-20 17:57       ` Florian Fainelli
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2021-07-20  2:32 UTC (permalink / raw)
  To: Jens Wiklander, Vikas Gupta, Rafał Miłecki
  Cc: Tyler Hicks, Allen Pais, Sumit Garg, Peter Huewe,
	Jarkko Sakkinen, Jason Gunthorpe, Thirupathaiah Annapureddy,
	Pavel Tatashin, OP-TEE TrustedFirmware, linux-integrity,
	BCM Kernel Feedback, linux-mips, Linux Kernel Mailing List



On 7/19/2021 3:49 AM, Jens Wiklander wrote:
> Hi,
> 
> On Fri, Jul 16, 2021 at 4:48 AM Vikas Gupta <vikas.gupta@broadcom.com> wrote:
>>
>> Hi Allen/Tyler,
>>   The patch looks good to me.
> 
> Thanks.
> 
> Rafal, is it OK if I include this patch together with the rest of the
> patches in this patch set in a pull request to arm-soc?

I can take those patches through the Broadcom ARM SoC pull request, 
Rafal would that work for you? We seem to have a bit of a maintainer 
coverage blind spot for that directory.
-- 
Florian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
  2021-07-20  2:32     ` Florian Fainelli
@ 2021-07-20 17:57       ` Florian Fainelli
  2021-07-20 18:15         ` Tyler Hicks
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2021-07-20 17:57 UTC (permalink / raw)
  To: Jens Wiklander, Vikas Gupta, Rafał Miłecki
  Cc: Tyler Hicks, Allen Pais, Sumit Garg, Peter Huewe,
	Jarkko Sakkinen, Jason Gunthorpe, Thirupathaiah Annapureddy,
	Pavel Tatashin, OP-TEE TrustedFirmware, linux-integrity,
	BCM Kernel Feedback, linux-mips, Linux Kernel Mailing List



On 7/19/2021 7:32 PM, Florian Fainelli wrote:
> 
> 
> On 7/19/2021 3:49 AM, Jens Wiklander wrote:
>> Hi,
>>
>> On Fri, Jul 16, 2021 at 4:48 AM Vikas Gupta <vikas.gupta@broadcom.com> 
>> wrote:
>>>
>>> Hi Allen/Tyler,
>>>   The patch looks good to me.
>>
>> Thanks.
>>
>> Rafal, is it OK if I include this patch together with the rest of the
>> patches in this patch set in a pull request to arm-soc?
> 
> I can take those patches through the Broadcom ARM SoC pull request, 
> Rafal would that work for you? We seem to have a bit of a maintainer 
> coverage blind spot for that directory.

Applied to drivers/fixes: 
https://github.com/Broadcom/stblinux/commit/4ecd797b7e16eb7f1b86fbfd7e4a7887b192535b
-- 
Florian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
  2021-07-20 17:57       ` Florian Fainelli
@ 2021-07-20 18:15         ` Tyler Hicks
  2021-07-20 18:59           ` Florian Fainelli
  0 siblings, 1 reply; 7+ messages in thread
From: Tyler Hicks @ 2021-07-20 18:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Jens Wiklander, Vikas Gupta, Rafał Miłecki, Allen Pais,
	Sumit Garg, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
	Thirupathaiah Annapureddy, Pavel Tatashin,
	OP-TEE TrustedFirmware, linux-integrity, BCM Kernel Feedback,
	linux-mips, Linux Kernel Mailing List

On 2021-07-20 10:57:18, Florian Fainelli wrote:
> 
> 
> On 7/19/2021 7:32 PM, Florian Fainelli wrote:
> > 
> > 
> > On 7/19/2021 3:49 AM, Jens Wiklander wrote:
> > > Hi,
> > > 
> > > On Fri, Jul 16, 2021 at 4:48 AM Vikas Gupta
> > > <vikas.gupta@broadcom.com> wrote:
> > > > 
> > > > Hi Allen/Tyler,
> > > >   The patch looks good to me.
> > > 
> > > Thanks.
> > > 
> > > Rafal, is it OK if I include this patch together with the rest of the
> > > patches in this patch set in a pull request to arm-soc?
> > 
> > I can take those patches through the Broadcom ARM SoC pull request,
> > Rafal would that work for you? We seem to have a bit of a maintainer
> > coverage blind spot for that directory.
> 
> Applied to drivers/fixes: https://github.com/Broadcom/stblinux/commit/4ecd797b7e16eb7f1b86fbfd7e4a7887b192535b

Thanks, Florian, but note that you won't be able to build that branch
since the commit uses a new function (tee_shm_alloc_kernel_buf()) that's
added earlier in the full series. It seems like it is going to be easier
for this to all go through Jens.

Tyler

> -- 
> Florian
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
  2021-07-20 18:15         ` Tyler Hicks
@ 2021-07-20 18:59           ` Florian Fainelli
  0 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2021-07-20 18:59 UTC (permalink / raw)
  To: Tyler Hicks, Florian Fainelli
  Cc: Jens Wiklander, Vikas Gupta, Rafał Miłecki, Allen Pais,
	Sumit Garg, Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe,
	Thirupathaiah Annapureddy, Pavel Tatashin,
	OP-TEE TrustedFirmware, linux-integrity, BCM Kernel Feedback,
	linux-mips, Linux Kernel Mailing List

On 7/20/21 11:15 AM, Tyler Hicks wrote:
> On 2021-07-20 10:57:18, Florian Fainelli wrote:
>>
>>
>> On 7/19/2021 7:32 PM, Florian Fainelli wrote:
>>>
>>>
>>> On 7/19/2021 3:49 AM, Jens Wiklander wrote:
>>>> Hi,
>>>>
>>>> On Fri, Jul 16, 2021 at 4:48 AM Vikas Gupta
>>>> <vikas.gupta@broadcom.com> wrote:
>>>>>
>>>>> Hi Allen/Tyler,
>>>>>   The patch looks good to me.
>>>>
>>>> Thanks.
>>>>
>>>> Rafal, is it OK if I include this patch together with the rest of the
>>>> patches in this patch set in a pull request to arm-soc?
>>>
>>> I can take those patches through the Broadcom ARM SoC pull request,
>>> Rafal would that work for you? We seem to have a bit of a maintainer
>>> coverage blind spot for that directory.
>>
>> Applied to drivers/fixes: https://github.com/Broadcom/stblinux/commit/4ecd797b7e16eb7f1b86fbfd7e4a7887b192535b
> 
> Thanks, Florian, but note that you won't be able to build that branch
> since the commit uses a new function (tee_shm_alloc_kernel_buf()) that's
> added earlier in the full series. It seems like it is going to be easier
> for this to all go through Jens.

I was grepping for the new functions added and could find all
references, though it looks like I missed tee_shm_alloc_kernel_buf()
somehow, so yes, having Jens merge that series all together would make
more sense here. If you need it:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec
  2021-06-14 22:33 [PATCH v5 0/8] tee: Improve support for kexec and kdump Tyler Hicks
@ 2021-06-14 22:33 ` Tyler Hicks
  0 siblings, 0 replies; 7+ messages in thread
From: Tyler Hicks @ 2021-06-14 22:33 UTC (permalink / raw)
  To: Jens Wiklander, Allen Pais, Sumit Garg, Peter Huewe,
	Jarkko Sakkinen, Jason Gunthorpe, Vikas Gupta
  Cc: Thirupathaiah Annapureddy, Pavel Tatashin,
	Rafał Miłecki, op-tee, linux-integrity,
	bcm-kernel-feedback-list, linux-mips, linux-kernel

From: Allen Pais <apais@linux.microsoft.com>

Implement a .shutdown hook that will be called during a kexec operation
so that the TEE shared memory, session, and context that were set up
during .probe can be properly freed/closed.

Additionally, don't use dma-buf backed shared memory for the
fw_shm_pool. dma-buf backed shared memory cannot be reliably freed and
unregistered during a kexec operation even when tee_shm_free() is called
on the shm from a .shutdown hook. The problem occurs because
dma_buf_put() calls fput() which then uses task_work_add(), with the
TWA_RESUME parameter, to queue tee_shm_release() to be called before the
current task returns to user mode. However, the current task never
returns to user mode before the kexec completes so the memory is never
freed nor unregistered.

Use tee_shm_alloc_kernel_buf() to avoid dma-buf backed shared memory
allocation so that tee_shm_free() can directly call tee_shm_release().
This will ensure that the shm can be freed and unregistered during a
kexec operation.

Fixes: 246880958ac9 ("firmware: broadcom: add OP-TEE based BNXT f/w manager")
Cc: stable@vger.kernel.org
Signed-off-by: Allen Pais <apais@linux.microsoft.com>
Co-developed-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/firmware/broadcom/tee_bnxt_fw.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/broadcom/tee_bnxt_fw.c b/drivers/firmware/broadcom/tee_bnxt_fw.c
index ed10da5313e8..a5bf4c3f6dc7 100644
--- a/drivers/firmware/broadcom/tee_bnxt_fw.c
+++ b/drivers/firmware/broadcom/tee_bnxt_fw.c
@@ -212,10 +212,9 @@ static int tee_bnxt_fw_probe(struct device *dev)
 
 	pvt_data.dev = dev;
 
-	fw_shm_pool = tee_shm_alloc(pvt_data.ctx, MAX_SHM_MEM_SZ,
-				    TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+	fw_shm_pool = tee_shm_alloc_kernel_buf(pvt_data.ctx, MAX_SHM_MEM_SZ);
 	if (IS_ERR(fw_shm_pool)) {
-		dev_err(pvt_data.dev, "tee_shm_alloc failed\n");
+		dev_err(pvt_data.dev, "tee_shm_alloc_kernel_buf failed\n");
 		err = PTR_ERR(fw_shm_pool);
 		goto out_sess;
 	}
@@ -242,6 +241,14 @@ static int tee_bnxt_fw_remove(struct device *dev)
 	return 0;
 }
 
+static void tee_bnxt_fw_shutdown(struct device *dev)
+{
+	tee_shm_free(pvt_data.fw_shm_pool);
+	tee_client_close_session(pvt_data.ctx, pvt_data.session_id);
+	tee_client_close_context(pvt_data.ctx);
+	pvt_data.ctx = NULL;
+}
+
 static const struct tee_client_device_id tee_bnxt_fw_id_table[] = {
 	{UUID_INIT(0x6272636D, 0x2019, 0x0716,
 		    0x42, 0x43, 0x4D, 0x5F, 0x53, 0x43, 0x48, 0x49)},
@@ -257,6 +264,7 @@ static struct tee_client_driver tee_bnxt_fw_driver = {
 		.bus		= &tee_bus_type,
 		.probe		= tee_bnxt_fw_probe,
 		.remove		= tee_bnxt_fw_remove,
+		.shutdown	= tee_bnxt_fw_shutdown,
 	},
 };
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-07-20 18:59 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210716022332.GC3232@sequoia>
2021-07-16  2:56 ` [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec Vikas Gupta
     [not found] ` <CAHLZf_t5U1bh1H8sULbJz7xrZ-r3Dcmxuw9MMmG2fehS3C72uQ@mail.gmail.com>
2021-07-19 10:49   ` Jens Wiklander
2021-07-20  2:32     ` Florian Fainelli
2021-07-20 17:57       ` Florian Fainelli
2021-07-20 18:15         ` Tyler Hicks
2021-07-20 18:59           ` Florian Fainelli
2021-06-14 22:33 [PATCH v5 0/8] tee: Improve support for kexec and kdump Tyler Hicks
2021-06-14 22:33 ` [PATCH v5 8/8] firmware: tee_bnxt: Release TEE shm, session, and context during kexec Tyler Hicks

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).