[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Stephen Boyd]

Sparse complains a bit on this file about endian issues and
__user casting:

arch/arm64/kernel/traps.c:87:37: warning: incorrect type in argument 1 (different address spaces)
arch/arm64/kernel/traps.c:87:37:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm64/kernel/traps.c:87:37:    got unsigned long *<noident>
arch/arm64/kernel/traps.c:116:23: warning: incorrect type in argument 1 (different address spaces)
arch/arm64/kernel/traps.c:116:23:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm64/kernel/traps.c:116:23:    got unsigned int [usertype] *
arch/arm64/kernel/traps.c:346:25: warning: cast to restricted __le16
arch/arm64/kernel/traps.c:352:34: warning: cast to restricted __le16
arch/arm64/kernel/traps.c:359:25: warning: cast to restricted __le32

Mark the types appropriately, and force the cast in get_user()
when assigning to 0 so sparse doesn't complain. The resulting
object code is the same before and after this commit.

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
---

Noticed while making other changes to this file. There are other issues still
about marking symbols static, but I'm not sure we want to introduce another
header file for the asmlinkage functions?

arch/arm64/kernel/traps.c:429:29: warning: symbol 'do_undefinstr' was not declared. Should it be static?
arch/arm64/kernel/traps.c:529:29: warning: symbol 'do_sysinstr' was not declared. Should it be static?
arch/arm64/kernel/traps.c:544:17: warning: symbol 'do_ni_syscall' was not declared. Should it be static?
arch/arm64/kernel/traps.c:615:17: warning: symbol 'bad_mode' was not declared. Should it be static?
arch/arm64/kernel/traps.c:632:17: warning: symbol 'bad_el0_sync' was not declared. Should it be static?
arch/arm64/kernel/traps.c:722:12: warning: symbol 'early_brk64' was not declared. Should it be static?
arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
arch/arm64/kernel/traps.c:568:10:   also defined here

 arch/arm64/include/asm/uaccess.h |  2 +-
 arch/arm64/kernel/traps.c        | 23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 46da3ea638bb..2f5b4ae98ee0 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -287,7 +287,7 @@ do {									\
 	might_fault();							\
 	access_ok(VERIFY_READ, __p, sizeof(*__p)) ?			\
 		__get_user((x), __p) :					\
-		((x) = 0, -EFAULT);					\
+		((x) = (__force __typeof__(*(ptr)))0, -EFAULT);		\
 })
 
 #define __put_user_asm(instr, alt_instr, reg, x, addr, err, feature)	\
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 659b2e6b6cf7..23959cb70ded 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 			if (p >= bottom && p < top) {
 				unsigned long val;
 
-				if (__get_user(val, (unsigned long *)p) == 0)
+				if (__get_user(val, (unsigned long __user *)p) == 0)
 					sprintf(str + i * 17, " %016lx", val);
 				else
 					sprintf(str + i * 17, " ????????????????");
@@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
 	for (i = -4; i < 1; i++) {
 		unsigned int val, bad;
 
-		bad = __get_user(val, &((u32 *)addr)[i]);
+		bad = __get_user(val, &((u32 __user *)addr)[i]);
 
 		if (!bad)
 			p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
@@ -340,23 +340,28 @@ static int call_undef_hook(struct pt_regs *regs)
 		return 1;
 
 	if (compat_thumb_mode(regs)) {
+		__le16 tinst;
+
 		/* 16-bit Thumb instruction */
-		if (get_user(instr, (u16 __user *)pc))
+		if (get_user(tinst, (__le16 __user *)pc))
 			goto exit;
-		instr = le16_to_cpu(instr);
+		instr = le16_to_cpu(tinst);
 		if (aarch32_insn_is_wide(instr)) {
-			u32 instr2;
+			__le16 tinstr2;
+			u16 instr2;
 
-			if (get_user(instr2, (u16 __user *)(pc + 2)))
+			if (get_user(tinstr2, (__le16 __user *)(pc + 2)))
 				goto exit;
-			instr2 = le16_to_cpu(instr2);
+			instr2 = le16_to_cpu(tinstr2);
 			instr = (instr << 16) | instr2;
 		}
 	} else {
+		__le32 ainst;
+
 		/* 32-bit ARM instruction */
-		if (get_user(instr, (u32 __user *)pc))
+		if (get_user(ainst, (__le32 __user *)pc))
 			goto exit;
-		instr = le32_to_cpu(instr);
+		instr = le32_to_cpu(ainst);
 	}
 
 	raw_spin_lock_irqsave(&undef_lock, flags);
-- 
2.10.0.297.gf6727b0

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Russell King - ARM Linux]

On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 659b2e6b6cf7..23959cb70ded 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -84,7 +84,7 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
>  			if (p >= bottom && p < top) {
>  				unsigned long val;
>  
> -				if (__get_user(val, (unsigned long *)p) == 0)
> +				if (__get_user(val, (unsigned long __user *)p) == 0)
>  					sprintf(str + i * 17, " %016lx", val);
>  				else
>  					sprintf(str + i * 17, " ????????????????");
> @@ -113,7 +113,7 @@ static void __dump_instr(const char *lvl, struct pt_regs *regs)
>  	for (i = -4; i < 1; i++) {
>  		unsigned int val, bad;
>  
> -		bad = __get_user(val, &((u32 *)addr)[i]);
> +		bad = __get_user(val, &((u32 __user *)addr)[i]);
>  
>  		if (!bad)
>  			p += sprintf(p, i == 0 ? "(%08x) " : "%08x ", val);
> @@ -340,23 +340,28 @@ static int call_undef_hook(struct pt_regs *regs)
>  		return 1;
>  
>  	if (compat_thumb_mode(regs)) {
> +		__le16 tinst;
> +
>  		/* 16-bit Thumb instruction */
> -		if (get_user(instr, (u16 __user *)pc))
> +		if (get_user(tinst, (__le16 __user *)pc))
>  			goto exit;
> -		instr = le16_to_cpu(instr);
> +		instr = le16_to_cpu(tinst);
>  		if (aarch32_insn_is_wide(instr)) {
> -			u32 instr2;
> +			__le16 tinstr2;
> +			u16 instr2;
>  
> -			if (get_user(instr2, (u16 __user *)(pc + 2)))
> +			if (get_user(tinstr2, (__le16 __user *)(pc + 2)))
>  				goto exit;
> -			instr2 = le16_to_cpu(instr2);
> +			instr2 = le16_to_cpu(tinstr2);
>  			instr = (instr << 16) | instr2;
>  		}
>  	} else {
> +		__le32 ainst;
> +
>  		/* 32-bit ARM instruction */
> -		if (get_user(instr, (u32 __user *)pc))
> +		if (get_user(ainst, (__le32 __user *)pc))
>  			goto exit;
> -		instr = le32_to_cpu(instr);
> +		instr = le32_to_cpu(ainst);

For the majority of causes, these are _not_ user addresses, they are
kernel addresses.  The use of get_user() at these locations is a way
to safely access these kernel addresses when something has gone wrong
without causing a further oops.

Annotating them with __user to shut up sparse is incorrect.  The point
with sparse is _not_ to end up with a warning free kernel, but for it
to warn where things are not quite right in terms of semantics.  These
warnings should stay.

So, the warnings about lack of __user should stay.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> Sparse complains a bit on this file about endian issues and
> __user casting:
> 
> arch/arm64/kernel/traps.c:87:37: warning: incorrect type in argument 1 (different address spaces)
> arch/arm64/kernel/traps.c:87:37:    expected void const volatile [noderef] <asn:1>*<noident>
> arch/arm64/kernel/traps.c:87:37:    got unsigned long *<noident>
> arch/arm64/kernel/traps.c:116:23: warning: incorrect type in argument 1 (different address spaces)
> arch/arm64/kernel/traps.c:116:23:    expected void const volatile [noderef] <asn:1>*<noident>
> arch/arm64/kernel/traps.c:116:23:    got unsigned int [usertype] *

The fact that __get_user() can and is used for both __kernel & __user pointers
defeat any sensible annotation. The proper way would be to have a special
version of __get_user() which would ignore the __user part of the pointer,
something like "__get_user_but_accept_any_pointer()" ...


> arch/arm64/kernel/traps.c:346:25: warning: cast to restricted __le16
> arch/arm64/kernel/traps.c:352:34: warning: cast to restricted __le16
> arch/arm64/kernel/traps.c:359:25: warning: cast to restricted __le32

Your patch looked correct to me for those.
 
> Mark the types appropriately, and force the cast in get_user()
> when assigning to 0 so sparse doesn't complain.
I didn't looked deeply at this one but I don't think it is needed.
Care to give more details?


> Noticed while making other changes to this file. There are other issues still
> about marking symbols static, but I'm not sure we want to introduce another
> header file for the asmlinkage functions?
Probably not, indeed.

> arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> arch/arm64/kernel/traps.c:568:10:   also defined here
This one I find strange. Can you tell which are those two entries?

 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 46da3ea638bb..2f5b4ae98ee0 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -287,7 +287,7 @@ do {									\
>  	might_fault();							\
>  	access_ok(VERIFY_READ, __p, sizeof(*__p)) ?			\
>  		__get_user((x), __p) :					\
> -		((x) = 0, -EFAULT);					\
> +		((x) = (__force __typeof__(*(ptr)))0, -EFAULT);		\
>  })

As said above, this one is dubious.


Luc Van Oostenryck

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Mon, Feb 20, 2017 at 9:47 AM, Stephen Boyd <stephen.boyd@linaro.org> wrote:
> Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
>> On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
>> > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
>> > arch/arm64/kernel/traps.c:568:10:   also defined here
>> This one I find strange. Can you tell which are those two entries?
>>
>
> This is:
>
>  static const char *esr_class_str[] = {
>          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
>          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
>
> where we initialize the entire array to an "unknown" value once, and
> then fill in the known values after that. This isn't a very common
> pattern, but it is used from time to time to avoid having lots of lines
> to do the same thing.

I see, yes can be handy but indeed spaese doesn't know about it yet.

Luc

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Mark Rutland]

On Mon, Feb 20, 2017 at 12:47:57AM -0800, Stephen Boyd wrote:
> Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> > On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> >  
> > > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > > arch/arm64/kernel/traps.c:568:10:   also defined here
> > This one I find strange. Can you tell which are those two entries?
> 
> This is:
> 
>  static const char *esr_class_str[] = {
>          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
>          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
> 
> where we initialize the entire array to an "unknown" value once, and
> then fill in the known values after that. This isn't a very common
> pattern, but it is used from time to time to avoid having lots of lines
> to do the same thing.

FWIW, it's a fairly common trick for syscall tables, which is where I
copied it from for the above. Certainly it's not that common elsewhere.

[mark@leverpostej:~/src/linux]% tail -n 11  arch/arm64/kernel/sys.c
#undef __SYSCALL
#define __SYSCALL(nr, sym)      [nr] = sym,

/*
 * The sys_call_table array must be 4K aligned to be accessible from
 * kernel/entry.S.
 */
void * const sys_call_table[__NR_syscalls] __aligned(4096) = {
        [0 ... __NR_syscalls - 1] = sys_ni_syscall,
#include <asm/unistd.h>
};

[mark@leverpostej:~/src/linux]% git grep '\[0 \.\.\. ' | grep sys
arch/arc/kernel/sys.c:  [0 ... NR_syscalls-1] = sys_ni_syscall,
arch/arm64/kernel/sys.c:        [0 ... __NR_syscalls - 1] = sys_ni_syscall,
arch/arm64/kernel/sys32.c:      [0 ... __NR_compat_syscalls - 1] = sys_ni_syscall,
arch/c6x/kernel/sys_c6x.c:      [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/metag/kernel/sys_metag.c:  [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/tile/kernel/compat.c:      [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/tile/kernel/sys.c: [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/unicore32/kernel/sys.c:    [0 ... __NR_syscalls-1] = sys_ni_syscall,
arch/x86/entry/syscall_32.c:    [0 ... __NR_syscall_compat_max] = &sys_ni_syscall,
arch/x86/entry/syscall_64.c:    [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/x86/um/sys_call_table_32.c:        [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/x86/um/sys_call_table_64.c:        [0 ... __NR_syscall_max] = &sys_ni_syscall,
arch/xtensa/kernel/syscall.c:   [0 ... __NR_syscall_count - 1] = (syscall_t)&sys_ni_syscall,

It would be nice to make sparse aware of this somehow, even if it
requires some annotation.

Thanks,
Mark.

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Mon, Feb 20, 2017 at 10:49:47AM +0000, Mark Rutland wrote:
> On Mon, Feb 20, 2017 at 12:47:57AM -0800, Stephen Boyd wrote:
> > Quoting Luc Van Oostenryck (2017-02-18 17:58:09)
> > > On Fri, Feb 17, 2017 at 08:51:12AM -0800, Stephen Boyd wrote:
> > >  
> > > > arch/arm64/kernel/traps.c:567:10: warning: Initializer entry defined twice
> > > > arch/arm64/kernel/traps.c:568:10:   also defined here
> > > This one I find strange. Can you tell which are those two entries?
> > 
> > This is:
> > 
> >  static const char *esr_class_str[] = {
> >          [0 ... ESR_ELx_EC_MAX]          = "UNRECOGNIZED EC",
> >          [ESR_ELx_EC_UNKNOWN]            = "Unknown/Uncategorized",
> > 
> > where we initialize the entire array to an "unknown" value once, and
> > then fill in the known values after that. This isn't a very common
> > pattern, but it is used from time to time to avoid having lots of lines
> > to do the same thing.
> 
> FWIW, it's a fairly common trick for syscall tables, which is where I
> copied it from for the above. Certainly it's not that common elsewhere.
> 
> ...
> 
> It would be nice to make sparse aware of this somehow, even if it
> requires some annotation.
> 
> Thanks,
> Mark.

I just checked this and I'm not very sure what's best.
Sparse is very well aware of the '...' to specify a range
in an array initializer or in switch-case. The warning
is there only because those entries are later overridden
with some value.
When checking what GCC is doing in this situation is saw
that by default even in cases like:
	static in ref[] = {
		[1] = 1,
		[2] = 2,
	};
GCC doesn't issue a warning. You need to use the flag
-Woverride-init for that. But then you also have a warning
for our current case:
	static in foo[] = {
		[0 ... 3] = 1,
		[0] = 2,
	};

It's easy enough to patch sparse to not issue a warning when the
override concerns a range (which would be perfect for the situation here),
Controlled or not by a new warning flag. But I'm far from convinced
that all uses of such "ranged-initialization" is used for default values
that may be later overridden.

I'm also reluctant to introduce yet another special annotation.

Any thoughts anyone about what we woudl like?


Note: sparse is quite incomplete regarding these overridden entries
  since it issues a warning only when the override is on the first
  element of the range. In others words, the range itself is not
  taken in account. So, no warnigs for code like:
	static in foo[] = {
		[0 ... 3] = 1,
		[1] = 2,
	};


-- Luc Van Oostenryck

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Will Deacon]

On Mon, Feb 20, 2017 at 10:33:45PM +0100, Luc Van Oostenryck wrote:
> I just checked this and I'm not very sure what's best.
> Sparse is very well aware of the '...' to specify a range
> in an array initializer or in switch-case. The warning
> is there only because those entries are later overridden
> with some value.
> When checking what GCC is doing in this situation is saw
> that by default even in cases like:
> 	static in ref[] = {
> 		[1] = 1,
> 		[2] = 2,
> 	};
> GCC doesn't issue a warning. You need to use the flag
> -Woverride-init for that. But then you also have a warning
> for our current case:
> 	static in foo[] = {
> 		[0 ... 3] = 1,
> 		[0] = 2,
> 	};
> 
> It's easy enough to patch sparse to not issue a warning when the
> override concerns a range (which would be perfect for the situation here),
> Controlled or not by a new warning flag. But I'm far from convinced
> that all uses of such "ranged-initialization" is used for default values
> that may be later overridden.

How about not warning only when the overridden range covers the entire
length of the array? The only broken case I can think of that slips
through the cracks then is if somebody typoed the range so that it
accidentally covered the whole array and therefore suppressed the override
warning.

Will

Re: [PATCH] arm64: traps: Mark __le16, __le32, __user variables properly

[Luc Van Oostenryck]

On Tue, Feb 21, 2017 at 11:03:56AM +0000, Will Deacon wrote:
> On Mon, Feb 20, 2017 at 10:33:45PM +0100, Luc Van Oostenryck wrote:
> > It's easy enough to patch sparse to not issue a warning when the
> > override concerns a range (which would be perfect for the situation here),
> > Controlled or not by a new warning flag. But I'm far from convinced
> > that all uses of such "ranged-initialization" is used for default values
> > that may be later overridden.
> 
> How about not warning only when the overridden range covers the entire
> length of the array? The only broken case I can think of that slips
> through the cracks then is if somebody typoed the range so that it
> accidentally covered the whole array and therefore suppressed the override
> warning.
> 
> Will

I like it. Patch is coming.

Luc