[PATCH] sparc64/gup: check address scope legitimacy

[PATCH] sparc64/gup: check address scope legitimacy

[Yang Shi]

Check if user address is accessible in atomic version __get_user_pages_fast()
before walking the page table.
And, check if end > start in get_user_pages_fast(), otherwise fallback to slow
path.

Signed-off-by: Yang Shi <yang.shi@linaro.org>
---
 arch/sparc/mm/gup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
index 2e5c4fc..08b8da5 100644
--- a/arch/sparc/mm/gup.c
+++ b/arch/sparc/mm/gup.c
@@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
 	end = start + len;
+	if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
+					(void __user *)start, len)))
+		return 0;
 
 	local_irq_save(flags);
 	pgdp = pgd_offset(mm, addr);
@@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
 	end = start + len;
+	if (end < start)
+		goto slow_irqon;
 
 	/*
 	 * XXX: batch / limit 'nr', to avoid large irq off latency
-- 
2.0.2

[V2 PATCH] sparc64/gup: check address scope legitimacy

[Yang Shi]

Check if user address is accessible in atomic version __get_user_pages_fast()
before walking the page table.
And, check if end > start in get_user_pages_fast(), otherwise fallback to slow
path.

Signed-off-by: Yang Shi <yang.shi@linaro.org>
---
Just found slow_irqon label is not defined, added it to avoid compile error.

 arch/sparc/mm/gup.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
index 2e5c4fc..cf4fb47 100644
--- a/arch/sparc/mm/gup.c
+++ b/arch/sparc/mm/gup.c
@@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
 	end = start + len;
+	if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
+					(void __user *)start, len)))
+		return 0;
 
 	local_irq_save(flags);
 	pgdp = pgd_offset(mm, addr);
@@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
 	end = start + len;
+	if (end < start)
+		goto slow_irqon;
 
 	/*
 	 * XXX: batch / limit 'nr', to avoid large irq off latency
@@ -244,7 +249,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 
 slow:
 		local_irq_enable();
-
+slow_irqon:
 		/* Try to get the remaining pages with get_user_pages */
 		start += nr << PAGE_SHIFT;
 		pages += nr;
-- 
2.0.2

Re: [V2 PATCH] sparc64/gup: check address scope legitimacy

[Sam Ravnborg]

Hi Yang.

On Wed, Nov 25, 2015 at 02:45:43PM -0800, Yang Shi wrote:
> Check if user address is accessible in atomic version __get_user_pages_fast()
> before walking the page table.
> And, check if end > start in get_user_pages_fast(), otherwise fallback to slow
> path.

Two different but related things in one patch is often a bad thing.
It would have been better to split it up.


> 
> Signed-off-by: Yang Shi <yang.shi@linaro.org>
> ---
> Just found slow_irqon label is not defined, added it to avoid compile error.
> 
>  arch/sparc/mm/gup.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
> index 2e5c4fc..cf4fb47 100644
> --- a/arch/sparc/mm/gup.c
> +++ b/arch/sparc/mm/gup.c
> @@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>  	addr = start;
>  	len = (unsigned long) nr_pages << PAGE_SHIFT;
>  	end = start + len;
> +	if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
> +					(void __user *)start, len)))
> +		return 0;
This change is not justified.
Why would we take the time to first do the access_ok() stuff.
If this had been an expensive operation then we had made this function
slower in the normal case ( assuming there were no access violations in the
normal case).
When I look at the implementation of access_ok() I get the impression that
this is not really a check we need.

access_ok() always returns 1.


>  
>  	local_irq_save(flags);
>  	pgdp = pgd_offset(mm, addr);
> @@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
>  	addr = start;
>  	len = (unsigned long) nr_pages << PAGE_SHIFT;
>  	end = start + len;
> +	if (end < start)
> +		goto slow_irqon;

end can only be smaller than start if there is some overflow.
See how end is calculated just the line above.

This looks like a highly suspicious change.

	Sam

Re: [V2 PATCH] sparc64/gup: check address scope legitimacy

[Shi, Yang]

On 12/3/2015 12:38 PM, Sam Ravnborg wrote:
> Hi Yang.
>
> On Wed, Nov 25, 2015 at 02:45:43PM -0800, Yang Shi wrote:
>> Check if user address is accessible in atomic version __get_user_pages_fast()
>> before walking the page table.
>> And, check if end > start in get_user_pages_fast(), otherwise fallback to slow
>> path.
>
> Two different but related things in one patch is often a bad thing.
> It would have been better to split it up.
>
>
>>
>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
>> ---
>> Just found slow_irqon label is not defined, added it to avoid compile error.
>>
>>   arch/sparc/mm/gup.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c
>> index 2e5c4fc..cf4fb47 100644
>> --- a/arch/sparc/mm/gup.c
>> +++ b/arch/sparc/mm/gup.c
>> @@ -173,6 +173,9 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>>   	addr = start;
>>   	len = (unsigned long) nr_pages << PAGE_SHIFT;
>>   	end = start + len;
>> +	if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
>> +					(void __user *)start, len)))
>> +		return 0;
> This change is not justified.
> Why would we take the time to first do the access_ok() stuff.
> If this had been an expensive operation then we had made this function
> slower in the normal case ( assuming there were no access violations in the
> normal case).
> When I look at the implementation of access_ok() I get the impression that
> this is not really a check we need.
>
> access_ok() always returns 1.

Thanks for pointing it out. And, I didn't notice that gup is just built 
for SPARC64. I though it is built by both 64 bit and 32 bit.

A follow-up question, is there any reason to just have sparc specific 
fast gup for 64 bit not for 32 bit?

>
>
>>
>>   	local_irq_save(flags);
>>   	pgdp = pgd_offset(mm, addr);
>> @@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
>>   	addr = start;
>>   	len = (unsigned long) nr_pages << PAGE_SHIFT;
>>   	end = start + len;
>> +	if (end < start)
>> +		goto slow_irqon;
>
> end can only be smaller than start if there is some overflow.
> See how end is calculated just the line above.
>
> This looks like a highly suspicious change.

I'm supposed this is used to protect the overflow. I copied the code 
from other arch. Actually, every arch has this except sparc.

Thanks,
Yang

>
> 	Sam
>

Re: [V2 PATCH] sparc64/gup: check address scope legitimacy

[Sam Ravnborg]

> >access_ok() always returns 1.
> 
> Thanks for pointing it out. And, I didn't notice that gup is just built for
> SPARC64. I though it is built by both 64 bit and 32 bit.
> 
> A follow-up question, is there any reason to just have sparc specific fast
> gup for 64 bit not for 32 bit?
I do not know - sorry.

> >>@@ -203,6 +206,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
> >>  	addr = start;
> >>  	len = (unsigned long) nr_pages << PAGE_SHIFT;
> >>  	end = start + len;
> >>+	if (end < start)
> >>+		goto slow_irqon;
> >
> >end can only be smaller than start if there is some overflow.
> >See how end is calculated just the line above.
> >
> >This looks like a highly suspicious change.
> 
> I'm supposed this is used to protect the overflow. I copied the code from
> other arch. Actually, every arch has this except sparc.
The the other archs are likely confused as well - there is most
likely some history behind that can be found if digging a little.

The code is not present in the generic version.

	Sam

Re: [PATCH] sparc64/gup: check address scope legitimacy

[kbuild test robot]

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

Hi Yang,

[auto build test ERROR on v4.4-rc2]
[also build test ERROR on next-20151124]

url:    https://github.com/0day-ci/linux/commits/Yang-Shi/sparc64-gup-check-address-scope-legitimacy/20151126-065342
config: sparc64-allnoconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All errors (new ones prefixed by >>):

   arch/sparc/mm/gup.c: In function 'get_user_pages_fast':
>> arch/sparc/mm/gup.c:210:3: error: label 'slow_irqon' used but not defined
      goto slow_irqon;
      ^

vim +/slow_irqon +210 arch/sparc/mm/gup.c

   204	
   205		start &= PAGE_MASK;
   206		addr = start;
   207		len = (unsigned long) nr_pages << PAGE_SHIFT;
   208		end = start + len;
   209		if (end < start)
 > 210			goto slow_irqon;
   211	
   212		/*
   213		 * XXX: batch / limit 'nr', to avoid large irq off latency

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 4808 bytes --]

Re: [PATCH] sparc64/gup: check address scope legitimacy

[Shi, Yang]

On 11/25/2015 4:26 PM, kbuild test robot wrote:
> Hi Yang,
>
> [auto build test ERROR on v4.4-rc2]
> [also build test ERROR on next-20151124]
>
> url:    https://github.com/0day-ci/linux/commits/Yang-Shi/sparc64-gup-check-address-scope-legitimacy/20151126-065342
> config: sparc64-allnoconfig (attached as .config)
> reproduce:
>          wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          make.cross ARCH=sparc64
>
> All errors (new ones prefixed by >>):
>
>     arch/sparc/mm/gup.c: In function 'get_user_pages_fast':
>>> arch/sparc/mm/gup.c:210:3: error: label 'slow_irqon' used but not defined
>        goto slow_irqon;

Already fixed in v2.

Thanks,
Yang

>        ^
>
> vim +/slow_irqon +210 arch/sparc/mm/gup.c
>
>     204	
>     205		start &= PAGE_MASK;
>     206		addr = start;
>     207		len = (unsigned long) nr_pages << PAGE_SHIFT;
>     208		end = start + len;
>     209		if (end < start)
>   > 210			goto slow_irqon;
>     211	
>     212		/*
>     213		 * XXX: batch / limit 'nr', to avoid large irq off latency
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>