[PATCH] tee: add overflow check in tee_ioctl_shm_register()

[PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Jens Wiklander]

commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.

With special lengths supplied by user space, tee_shm_register() has
an integer overflow when calculating the number of pages covered by a
supplied user space memory region.

This may cause pin_user_pages_fast() to do a NULL pointer dereference.

Fix this by adding an an explicit call to access_ok() in
tee_ioctl_shm_register() to catch an invalid user space address early.

Fixes: 033ddf12bcf5 ("tee: add register user memory")
Cc: stable@vger.kernel.org # 5.4
Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[JW: backport to stable-5.4 + update commit message]
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index a7ccd4d2bd10..2db144d2d26f 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 	if (data.flags)
 		return -EINVAL;
 
+	if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
+		return -EFAULT;
+
 	shm = tee_shm_register(ctx, data.addr, data.length,
 			       TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
 	if (IS_ERR(shm))
-- 
2.31.1

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg KH]

On Mon, Aug 22, 2022 at 11:26:21AM +0200, Jens Wiklander wrote:
> commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> 
> With special lengths supplied by user space, tee_shm_register() has
> an integer overflow when calculating the number of pages covered by a
> supplied user space memory region.
> 
> This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> 
> Fix this by adding an an explicit call to access_ok() in
> tee_ioctl_shm_register() to catch an invalid user space address early.
> 
> Fixes: 033ddf12bcf5 ("tee: add register user memory")
> Cc: stable@vger.kernel.org # 5.4
> Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [JW: backport to stable-5.4 + update commit message]

Will this also work for 4.19?

thanks,

greg k-h

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg KH]

On Mon, Aug 22, 2022 at 11:59:17AM +0200, Greg KH wrote:
> On Mon, Aug 22, 2022 at 11:26:21AM +0200, Jens Wiklander wrote:
> > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> > 
> > With special lengths supplied by user space, tee_shm_register() has
> > an integer overflow when calculating the number of pages covered by a
> > supplied user space memory region.
> > 
> > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> > 
> > Fix this by adding an an explicit call to access_ok() in
> > tee_ioctl_shm_register() to catch an invalid user space address early.
> > 
> > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > Cc: stable@vger.kernel.org # 5.4
> > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > [JW: backport to stable-5.4 + update commit message]
> 
> Will this also work for 4.19?

Nope, it breaks the build on 4.19.y, needs a different backport there:

  CC [M]  drivers/tee/tee_core.o
drivers/tee/tee_core.c: In function ‘tee_ioctl_shm_register’:
drivers/tee/tee_core.c:178:76: error: macro "access_ok" requires 3 arguments, but only 2 given
  178 |         if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
      |                                                                            ^
In file included from ./include/linux/uaccess.h:14,
                 from drivers/tee/tee_core.c:24:
./arch/x86/include/asm/uaccess.h:98: note: macro "access_ok" defined here
   98 | #define access_ok(type, addr, size)                                     \
      |
drivers/tee/tee_core.c:178:14: error: ‘access_ok’ undeclared (first use in this function)
  178 |         if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
      |              ^~~~~~~~~

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Jens Wiklander]

On Mon, Aug 22, 2022 at 12:01 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 22, 2022 at 11:59:17AM +0200, Greg KH wrote:
> > On Mon, Aug 22, 2022 at 11:26:21AM +0200, Jens Wiklander wrote:
> > > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> > >
> > > With special lengths supplied by user space, tee_shm_register() has
> > > an integer overflow when calculating the number of pages covered by a
> > > supplied user space memory region.
> > >
> > > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> > >
> > > Fix this by adding an an explicit call to access_ok() in
> > > tee_ioctl_shm_register() to catch an invalid user space address early.
> > >
> > > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > > Cc: stable@vger.kernel.org # 5.4
> > > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > [JW: backport to stable-5.4 + update commit message]
> >
> > Will this also work for 4.19?
>
> Nope, it breaks the build on 4.19.y, needs a different backport there:

Yeah, I'm just about to post a 4.19.y backport too.

Cheers,
Jens

>
>   CC [M]  drivers/tee/tee_core.o
> drivers/tee/tee_core.c: In function ‘tee_ioctl_shm_register’:
> drivers/tee/tee_core.c:178:76: error: macro "access_ok" requires 3 arguments, but only 2 given
>   178 |         if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
>       |                                                                            ^
> In file included from ./include/linux/uaccess.h:14,
>                  from drivers/tee/tee_core.c:24:
> ./arch/x86/include/asm/uaccess.h:98: note: macro "access_ok" defined here
>    98 | #define access_ok(type, addr, size)                                     \
>       |
> drivers/tee/tee_core.c:178:14: error: ‘access_ok’ undeclared (first use in this function)
>   178 |         if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
>       |              ^~~~~~~~~
>
>

[PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Jens Wiklander]

commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.

With special lengths supplied by user space, tee_shm_register() has
an integer overflow when calculating the number of pages covered by a
supplied user space memory region.

This may cause pin_user_pages_fast() to do a NULL pointer dereference.

Fix this by adding an an explicit call to access_ok() in
tee_ioctl_shm_register() to catch an invalid user space address early.

Fixes: 033ddf12bcf5 ("tee: add register user memory")
Cc: stable@vger.kernel.org # 5.4
Cc: stable@vger.kernel.org # 5.10
Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[JW: backport to stable 5.4 and 5.10 + update commit message]
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index a7ccd4d2bd10..2db144d2d26f 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 	if (data.flags)
 		return -EINVAL;
 
+	if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
+		return -EFAULT;
+
 	shm = tee_shm_register(ctx, data.addr, data.length,
 			       TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
 	if (IS_ERR(shm))
-- 
2.31.1

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg KH]

On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
> commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> 
> With special lengths supplied by user space, tee_shm_register() has
> an integer overflow when calculating the number of pages covered by a
> supplied user space memory region.
> 
> This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> 
> Fix this by adding an an explicit call to access_ok() in
> tee_ioctl_shm_register() to catch an invalid user space address early.
> 
> Fixes: 033ddf12bcf5 ("tee: add register user memory")
> Cc: stable@vger.kernel.org # 5.4
> Cc: stable@vger.kernel.org # 5.10
> Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [JW: backport to stable 5.4 and 5.10 + update commit message]

You already sent me a 5.4 version here:
	https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org

And I applied that.

And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add
overflow check in register_shm_helper()") and was in the 5.10.137
release.

> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index a7ccd4d2bd10..2db144d2d26f 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>  	if (data.flags)
>  		return -EINVAL;
>  
> +	if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
> +		return -EFAULT;

What I took in 5.10.137 was:

+       if (!access_ok((void __user *)addr, length))
+               return ERR_PTR(-EFAULT);

Should I fix it up to look like what you sent here instead?

confused,

greg k-h

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Jens Wiklander]

On Mon, Aug 22, 2022 at 3:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
> > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> >
> > With special lengths supplied by user space, tee_shm_register() has
> > an integer overflow when calculating the number of pages covered by a
> > supplied user space memory region.
> >
> > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> >
> > Fix this by adding an an explicit call to access_ok() in
> > tee_ioctl_shm_register() to catch an invalid user space address early.
> >
> > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > Cc: stable@vger.kernel.org # 5.4
> > Cc: stable@vger.kernel.org # 5.10
> > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > [JW: backport to stable 5.4 and 5.10 + update commit message]
>
> You already sent me a 5.4 version here:
>         https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
>
> And I applied that.
>
> And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add
> overflow check in register_shm_helper()") and was in the 5.10.137
> release.
>
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/tee_core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index a7ccd4d2bd10..2db144d2d26f 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
> >       if (data.flags)
> >               return -EINVAL;
> >
> > +     if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
> > +             return -EFAULT;
>
> What I took in 5.10.137 was:
>
> +       if (!access_ok((void __user *)addr, length))
> +               return ERR_PTR(-EFAULT);
>
> Should I fix it up to look like what you sent here instead?

Yes, please.

>
> confused,

I'm sorry for the confusion.

Thanks,
Jens

>
> greg k-h

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg KH]

On Mon, Aug 22, 2022 at 04:29:05PM +0200, Jens Wiklander wrote:
> On Mon, Aug 22, 2022 at 3:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
> > > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> > >
> > > With special lengths supplied by user space, tee_shm_register() has
> > > an integer overflow when calculating the number of pages covered by a
> > > supplied user space memory region.
> > >
> > > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> > >
> > > Fix this by adding an an explicit call to access_ok() in
> > > tee_ioctl_shm_register() to catch an invalid user space address early.
> > >
> > > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > > Cc: stable@vger.kernel.org # 5.4
> > > Cc: stable@vger.kernel.org # 5.10
> > > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > [JW: backport to stable 5.4 and 5.10 + update commit message]
> >
> > You already sent me a 5.4 version here:
> >         https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
> >
> > And I applied that.
> >
> > And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add
> > overflow check in register_shm_helper()") and was in the 5.10.137
> > release.
> >
> > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > ---
> > >  drivers/tee/tee_core.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > > index a7ccd4d2bd10..2db144d2d26f 100644
> > > --- a/drivers/tee/tee_core.c
> > > +++ b/drivers/tee/tee_core.c
> > > @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
> > >       if (data.flags)
> > >               return -EINVAL;
> > >
> > > +     if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
> > > +             return -EFAULT;
> >
> > What I took in 5.10.137 was:
> >
> > +       if (!access_ok((void __user *)addr, length))
> > +               return ERR_PTR(-EFAULT);
> >
> > Should I fix it up to look like what you sent here instead?
> 
> Yes, please.

Ok, no, that does not work on 5.10.y at all, it blows up with the
obvious issue that there is no data pointer in this function.  It's also
in a different file, drivers/tee/tee_shm.c

So I'm going to leave 5.10.y alone for now, I think it's fixed.

I've taken your 5.4 patch that you sent previously, and still need a
4.19.y change (and any older kernels that are also vulnerable.)

thanks,

greg k-h

[PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Jens Wiklander]

commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.

With special lengths supplied by user space, tee_shm_register() has
an integer overflow when calculating the number of pages covered by a
supplied user space memory region.

This may cause pin_user_pages_fast() to do a NULL pointer dereference.

Fix this by adding an an explicit call to access_ok() in
tee_ioctl_shm_register() to catch an invalid user space address early.

Fixes: 033ddf12bcf5 ("tee: add register user memory")
Cc: stable@vger.kernel.org # 4.19
Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
[JW: backport to stable-4.19 + update commit message]
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
---
 drivers/tee/tee_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index d42fc2ae8592..e568cb4b2ffc 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -175,6 +175,10 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 	if (data.flags)
 		return -EINVAL;
 
+	if (!access_ok(VERIFY_WRITE, (void __user *)(unsigned long)data.addr,
+		       data.length))
+		return -EFAULT;
+
 	shm = tee_shm_register(ctx, data.addr, data.length,
 			       TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
 	if (IS_ERR(shm))
-- 
2.31.1

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg KH]

On Mon, Aug 22, 2022 at 05:02:53PM +0200, Jens Wiklander wrote:
> commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> 
> With special lengths supplied by user space, tee_shm_register() has
> an integer overflow when calculating the number of pages covered by a
> supplied user space memory region.
> 
> This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> 
> Fix this by adding an an explicit call to access_ok() in
> tee_ioctl_shm_register() to catch an invalid user space address early.
> 
> Fixes: 033ddf12bcf5 ("tee: add register user memory")
> Cc: stable@vger.kernel.org # 4.19
> Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [JW: backport to stable-4.19 + update commit message]
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index d42fc2ae8592..e568cb4b2ffc 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -175,6 +175,10 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>  	if (data.flags)
>  		return -EINVAL;
>  
> +	if (!access_ok(VERIFY_WRITE, (void __user *)(unsigned long)data.addr,
> +		       data.length))
> +		return -EFAULT;
> +
>  	shm = tee_shm_register(ctx, data.addr, data.length,
>  			       TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
>  	if (IS_ERR(shm))
> -- 
> 2.31.1
> 

Now queued up, thanks.

greg k-h

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Jens Wiklander]

On Mon, Aug 22, 2022 at 4:57 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Aug 22, 2022 at 04:29:05PM +0200, Jens Wiklander wrote:
> > On Mon, Aug 22, 2022 at 3:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
> > > > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> > > >
> > > > With special lengths supplied by user space, tee_shm_register() has
> > > > an integer overflow when calculating the number of pages covered by a
> > > > supplied user space memory region.
> > > >
> > > > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> > > >
> > > > Fix this by adding an an explicit call to access_ok() in
> > > > tee_ioctl_shm_register() to catch an invalid user space address early.
> > > >
> > > > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > > > Cc: stable@vger.kernel.org # 5.4
> > > > Cc: stable@vger.kernel.org # 5.10
> > > > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > > > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > > > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > > > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > [JW: backport to stable 5.4 and 5.10 + update commit message]
> > >
> > > You already sent me a 5.4 version here:
> > >         https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
> > >
> > > And I applied that.
> > >
> > > And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add
> > > overflow check in register_shm_helper()") and was in the 5.10.137
> > > release.
> > >
> > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > ---
> > > >  drivers/tee/tee_core.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > > > index a7ccd4d2bd10..2db144d2d26f 100644
> > > > --- a/drivers/tee/tee_core.c
> > > > +++ b/drivers/tee/tee_core.c
> > > > @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
> > > >       if (data.flags)
> > > >               return -EINVAL;
> > > >
> > > > +     if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
> > > > +             return -EFAULT;
> > >
> > > What I took in 5.10.137 was:
> > >
> > > +       if (!access_ok((void __user *)addr, length))
> > > +               return ERR_PTR(-EFAULT);
> > >
> > > Should I fix it up to look like what you sent here instead?
> >
> > Yes, please.
>
> Ok, no, that does not work on 5.10.y at all, it blows up with the
> obvious issue that there is no data pointer in this function.  It's also
> in a different file, drivers/tee/tee_shm.c
>
> So I'm going to leave 5.10.y alone for now, I think it's fixed.

It works somewhat, but there's the potential memory leak that Pavel
Machek pointed out,
https://lore.kernel.org/lkml/20220822111546.GA7795@duo.ucw.cz/ .

The 5.4 patch has a better approach since it verifies the supplied
address range early before we do anything that must be undone on
error.
The 5.4 patch changes  tee_ioctl_shm_register() instead, which is the
function one step up in the call chain. This approach should be taken
for all kernels before 53e16519c2ec ("tee: replace
tee_shm_register()"), that is, before v5.18 if I'm reading the git log
correctly.

access_ok() went from taking three arguments to two sometime after
v4.19, why that patch is slightly different.

>
> I've taken your 5.4 patch that you sent previously, and still need a
> 4.19.y change (and any older kernels that are also vulnerable.)

The oldest vulnerable kernel is v4.16.

Thanks,
Jens

>
> thanks,
>
> greg k-h

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg KH]

On Tue, Aug 23, 2022 at 09:00:43AM +0200, Jens Wiklander wrote:
> On Mon, Aug 22, 2022 at 4:57 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Aug 22, 2022 at 04:29:05PM +0200, Jens Wiklander wrote:
> > > On Mon, Aug 22, 2022 at 3:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Mon, Aug 22, 2022 at 03:12:27PM +0200, Jens Wiklander wrote:
> > > > > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> > > > >
> > > > > With special lengths supplied by user space, tee_shm_register() has
> > > > > an integer overflow when calculating the number of pages covered by a
> > > > > supplied user space memory region.
> > > > >
> > > > > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> > > > >
> > > > > Fix this by adding an an explicit call to access_ok() in
> > > > > tee_ioctl_shm_register() to catch an invalid user space address early.
> > > > >
> > > > > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > > > > Cc: stable@vger.kernel.org # 5.4
> > > > > Cc: stable@vger.kernel.org # 5.10
> > > > > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > > > > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > > > > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > > > > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > > [JW: backport to stable 5.4 and 5.10 + update commit message]
> > > >
> > > > You already sent me a 5.4 version here:
> > > >         https://lore.kernel.org/r/20220822092621.3691771-1-jens.wiklander@linaro.org
> > > >
> > > > And I applied that.
> > > >
> > > > And for 5.10, it's already in the tree as commit 578c349570d2 ("tee: add
> > > > overflow check in register_shm_helper()") and was in the 5.10.137
> > > > release.
> > > >
> > > > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > > > > ---
> > > > >  drivers/tee/tee_core.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > > > > index a7ccd4d2bd10..2db144d2d26f 100644
> > > > > --- a/drivers/tee/tee_core.c
> > > > > +++ b/drivers/tee/tee_core.c
> > > > > @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
> > > > >       if (data.flags)
> > > > >               return -EINVAL;
> > > > >
> > > > > +     if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
> > > > > +             return -EFAULT;
> > > >
> > > > What I took in 5.10.137 was:
> > > >
> > > > +       if (!access_ok((void __user *)addr, length))
> > > > +               return ERR_PTR(-EFAULT);
> > > >
> > > > Should I fix it up to look like what you sent here instead?
> > >
> > > Yes, please.
> >
> > Ok, no, that does not work on 5.10.y at all, it blows up with the
> > obvious issue that there is no data pointer in this function.  It's also
> > in a different file, drivers/tee/tee_shm.c
> >
> > So I'm going to leave 5.10.y alone for now, I think it's fixed.
> 
> It works somewhat, but there's the potential memory leak that Pavel
> Machek pointed out,
> https://lore.kernel.org/lkml/20220822111546.GA7795@duo.ucw.cz/ .
> 
> The 5.4 patch has a better approach since it verifies the supplied
> address range early before we do anything that must be undone on
> error.
> The 5.4 patch changes  tee_ioctl_shm_register() instead, which is the
> function one step up in the call chain. This approach should be taken
> for all kernels before 53e16519c2ec ("tee: replace
> tee_shm_register()"), that is, before v5.18 if I'm reading the git log
> correctly.
> 
> access_ok() went from taking three arguments to two sometime after
> v4.19, why that patch is slightly different.

Ok, can you send me a new fix-up patch, on top of the latest 5.10.y
release, to resolve the 5.10 issue?

thanks,

greg k-h

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Sumit Garg]

Hi Greg,

On Mon, 22 Aug 2022 at 18:42, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
>
> With special lengths supplied by user space, tee_shm_register() has
> an integer overflow when calculating the number of pages covered by a
> supplied user space memory region.
>
> This may cause pin_user_pages_fast() to do a NULL pointer dereference.
>
> Fix this by adding an an explicit call to access_ok() in
> tee_ioctl_shm_register() to catch an invalid user space address early.
>
> Fixes: 033ddf12bcf5 ("tee: add register user memory")
> Cc: stable@vger.kernel.org # 5.4
> Cc: stable@vger.kernel.org # 5.10
> Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [JW: backport to stable 5.4 and 5.10 + update commit message]
> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  drivers/tee/tee_core.c | 3 +++
>  1 file changed, 3 insertions(+)
>

The v5.15 backport [1] for this fix has broken the kernel consumers
for tee_shm_register(), the trusted keys driver is one of them
reported here [2]. We need to fix that up with the following change
[3]. Would you like to revert the backport and apply the correct one
or should I prepare a fix patch for the following [3]?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.15.y&id=c12f0e6126ad223806a365084e86370511654bf1

[2] https://github.com/OP-TEE/optee_os/issues/5624

[3]

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 3fc426dad2df..d5c4fcd8733d 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -334,6 +334,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
        if (data.flags)
                return -EINVAL;

+       if (!access_ok((void __user *)data.addr, data.length))
+               return ERR_PTR(-EFAULT);
+
        shm = tee_shm_register(ctx, data.addr, data.length,
                               TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
        if (IS_ERR(shm))
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index bd96ebb82c8e..6fb4400333fb 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -223,9 +223,6 @@ struct tee_shm *tee_shm_register(struct
tee_context *ctx, unsigned long addr,
                goto err;
        }

-       if (!access_ok((void __user *)addr, length))
-               return ERR_PTR(-EFAULT);
-
        mutex_lock(&teedev->mutex);
        shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
        mutex_unlock(&teedev->mutex);

-Sumit

> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index a7ccd4d2bd10..2db144d2d26f 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -182,6 +182,9 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>         if (data.flags)
>                 return -EINVAL;
>
> +       if (!access_ok((void __user *)(unsigned long)data.addr, data.length))
> +               return -EFAULT;
> +
>         shm = tee_shm_register(ctx, data.addr, data.length,
>                                TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
>         if (IS_ERR(shm))
> --
> 2.31.1
>

Re: [PATCH] tee: add overflow check in tee_ioctl_shm_register()

[Greg Kroah-Hartman]

On Tue, Nov 08, 2022 at 11:42:01AM +0530, Sumit Garg wrote:
> Hi Greg,
> 
> On Mon, 22 Aug 2022 at 18:42, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > commit 573ae4f13f630d6660008f1974c0a8a29c30e18a upstream.
> >
> > With special lengths supplied by user space, tee_shm_register() has
> > an integer overflow when calculating the number of pages covered by a
> > supplied user space memory region.
> >
> > This may cause pin_user_pages_fast() to do a NULL pointer dereference.
> >
> > Fix this by adding an an explicit call to access_ok() in
> > tee_ioctl_shm_register() to catch an invalid user space address early.
> >
> > Fixes: 033ddf12bcf5 ("tee: add register user memory")
> > Cc: stable@vger.kernel.org # 5.4
> > Cc: stable@vger.kernel.org # 5.10
> > Reported-by: Nimish Mishra <neelam.nimish@gmail.com>
> > Reported-by: Anirban Chakraborty <ch.anirban00727@gmail.com>
> > Reported-by: Debdeep Mukhopadhyay <debdeep.mukhopadhyay@gmail.com>
> > Suggested-by: Jerome Forissier <jerome.forissier@linaro.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > [JW: backport to stable 5.4 and 5.10 + update commit message]
> > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
> > ---
> >  drivers/tee/tee_core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> 
> The v5.15 backport [1] for this fix has broken the kernel consumers
> for tee_shm_register(), the trusted keys driver is one of them
> reported here [2]. We need to fix that up with the following change
> [3]. Would you like to revert the backport and apply the correct one
> or should I prepare a fix patch for the following [3]?

A fixup patch is fine if needed, along with the description of why the
backport was broken.  Note, this commit went much further back than
5.15, so be sure to check older kernels too.

thanks,

greg k-h