[PATCH] optee: remove address tag in check_mem_type()

[PATCH] optee: remove address tag in check_mem_type()

[Volodymyr Babchuk]

Before passing 'start' to find_vma() we need to remove
tags from it to get sane results.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 drivers/tee/optee/call.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
index a5afbe6dee68..61420395137b 100644
--- a/drivers/tee/optee/call.c
+++ b/drivers/tee/optee/call.c
@@ -562,6 +562,8 @@ static int check_mem_type(unsigned long start, size_t num_pages)
 	struct mm_struct *mm = current->mm;
 	int rc;
 
+	start = untagged_addr(start);
+
 	down_read(&mm->mmap_sem);
 	rc = __check_mem_type(find_vma(mm, start),
 			      start + num_pages * PAGE_SIZE);
-- 
2.27.0

Re: [PATCH] optee: remove address tag in check_mem_type()

[Jens Wiklander]

On Mon, Oct 12, 2020 at 11:26 AM Volodymyr Babchuk
<Volodymyr_Babchuk@epam.com> wrote:
>
> Before passing 'start' to find_vma() we need to remove
> tags from it to get sane results.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  drivers/tee/optee/call.c | 2 ++
>  1 file changed, 2 insertions(+)

Would you mind giving a bit more background to this? For example in
which contexts this function does or doesn't work as expected? Do you
have any special use cases that don't work, etc? This is not a new
regression, it's rather a problem we've always had, right?

Thanks,
Jens

>
> diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> index a5afbe6dee68..61420395137b 100644
> --- a/drivers/tee/optee/call.c
> +++ b/drivers/tee/optee/call.c
> @@ -562,6 +562,8 @@ static int check_mem_type(unsigned long start, size_t num_pages)
>         struct mm_struct *mm = current->mm;
>         int rc;
>
> +       start = untagged_addr(start);
> +
>         down_read(&mm->mmap_sem);
>         rc = __check_mem_type(find_vma(mm, start),
>                               start + num_pages * PAGE_SIZE);
> --
> 2.27.0

Re: [PATCH] optee: remove address tag in check_mem_type()

[Volodymyr Babchuk]

Hello Jens,

Jens Wiklander writes:

> On Mon, Oct 12, 2020 at 11:26 AM Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com> wrote:
>>
>> Before passing 'start' to find_vma() we need to remove
>> tags from it to get sane results.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  drivers/tee/optee/call.c | 2 ++
>>  1 file changed, 2 insertions(+)
>
> Would you mind giving a bit more background to this? For example in
> which contexts this function does or doesn't work as expected? Do you
> have any special use cases that don't work, etc? This is not a new
> regression, it's rather a problem we've always had, right?

Yes, sorry. I had to clarify in the commit description. Issue was found
on Android. Android uses pointer tagging [1], so MSB of user pointers
contain tags. As a result, passing raw user address to find_vma() leads
to NULL result, as it only traverses RB tree and does not alter passed
address in any way.

Code in mm/gup.c already strips tags and maybe, it is better to call
untagged_addr() inside of find_vma(). I'm not sure. Probably, we need
some help from MM maintainers.

Anyways, this patched fixed issue with register_shm failing in our use
case.

[1] https://source.android.com/devices/tech/debug/tagged-pointers


-- 
Volodymyr Babchuk at EPAM