Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/13 18:51, Russell King (Oracle) wrote:
> On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote:
>> On 2022/10/10 19:06, Ard Biesheuvel wrote:
>>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
>>> <thunder.leizhen@xxxxxxxxxx> wrote:
>>>> On 2022/10/10 18:20, Ard Biesheuvel wrote:
>>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@xxxxxxxxxx> wrote:
>>>>>>
>>>>>> Fix the following warnings:
>>>>>>  warning: incorrect type in initializer (different address spaces)
>>>>>>     expected unsigned short [noderef] __user *register __p
>>>>>>     got unsigned short [usertype] *
>>>>>>  warning: cast removes address space '__user' of expression
>>>>>>
>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
>>>>>> ---
>>>>>>  arch/arm/kernel/traps.c | 10 +++++-----
>>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
>>>>>> --- a/arch/arm/kernel/traps.c
>>>>>> +++ b/arch/arm/kernel/traps.c
>>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>>>>                         }
>>>>>>                 } else {
>>>>>>                         if (thumb)
>>>>>> -                               bad = get_user(val, &((u16 *)addr)[i]);
>>>>>> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
>>>>>>                         else
>>>>>> -                               bad = get_user(val, &((u32 *)addr)[i]);
>>>>>> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
>>>>>>                 }
>>>>>>
>>>>>>                 if (!bad)
>>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>>>>>         if (processor_mode(regs) == SVC_MODE) {
>>>>>>  #ifdef CONFIG_THUMB2_KERNEL
>>>>>>                 if (thumb_mode(regs)) {
>>>>>> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>>>> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
>>>>>
>>>>> Shouldn't this be __user as well? (and below)
>>>>
>>>> unsigned int instr;
>>>> void __user *pc;
>>>>
>>>> The __user can clear the warning, but a new warning will be generated.
>>>>
>>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>>       ^new                           ^old
>>>>
>>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
>>>>
>>>
>>> This is because dereferencing a __user pointer is not permitted.
>>>
>>> So this code should be using get_kernel_nofault() here not a plain
>>> dereference of PC. So better to fix that properly instead of papering
>>> over it with a __force cast just to make sparse happy.
>>
>> How about:
>> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>>  {
>>         unsigned int instr;
>> -       void __user *pc;
>> +       void *pc;
>>
>> -       pc = (void __user *)instruction_pointer(regs);
>> +       pc = (void *)instruction_pointer(regs);
>>
>>         if (processor_mode(regs) == SVC_MODE) {
>>  #ifdef CONFIG_THUMB2_KERNEL
>> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>         }
>>  #endif
>>         arm_notify_die("Oops - undefined instruction", regs,
>> -                      SIGILL, ILL_ILLOPC, pc, 0, 6);
>> +                      SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>>  }
>>  NOKPROBE_SYMBOL(do_undefinstr)
>>
>>
>> The 'pc' may come from kernel or user. And I found that all the get_user()
>> calls have already done type casts to the pc, except arm_notify_die().
>> I think the above changes are reasonable.
> 
> If we're going to do that, lets do it properly - I think the above would
> need some __force usage to stop sparse complaining, whereas I don't
> think this will (untested):

The following changes are a little too big, and the above changes have solved
the problem completely. In order to focus on the main target, I removed this
part of the modification in v4. I hope you'll take the time to review v4.

> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3f468ac98592..827cbc022900 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>  {
>  	unsigned int instr;
> -	void __user *pc;
> +	unsigned long pc;

The following changes need to be added:
@@ -487,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
 die_sig:
 #ifdef CONFIG_DEBUG_USER
        if (user_debug & UDBG_UNDEFINED) {
-               pr_info("%s (%d): undefined instruction: pc=%px\n",
+               pr_info("%s (%d): undefined instruction: pc=%lx\n",
                        current->comm, task_pid_nr(current), pc);
                __show_regs(regs);
                dump_instr(KERN_INFO, regs);
>  
> -	pc = (void __user *)instruction_pointer(regs);
> +	pc = instruction_pointer(regs);
>  
>  	if (processor_mode(regs) == SVC_MODE) {
> -#ifdef CONFIG_THUMB2_KERNEL
> -		if (thumb_mode(regs)) {
> -			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> +		if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) {
> +			u16 *tpc = (u16 *)pc;
> +
> +			instr = __mem_to_opcode_thumb16(tpc[0]);
>  			if (is_wide_instruction(instr)) {
>  				u16 inst2;
> -				inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
> +
> +				inst2 = __mem_to_opcode_thumb16(tpc[1]);
>  				instr = __opcode_thumb32_compose(instr, inst2);
>  			}
> -		} else
> -#endif
> -			instr = __mem_to_opcode_arm(*(u32 *) pc);
> +		} else {
> +			u32 *apc = (u32 *)pc;
> +
> +			instr = __mem_to_opcode_arm(*apc);
> +		}
>  	} else if (thumb_mode(regs)) {
> -		if (get_user(instr, (u16 __user *)pc))
> +		u16 __user *tpc = (u16 __user *)pc;
> +
> +		if (get_user(instr, tpc))
>  			goto die_sig;
>  		instr = __mem_to_opcode_thumb16(instr);
>  		if (is_wide_instruction(instr)) {
>  			unsigned int instr2;
> -			if (get_user(instr2, (u16 __user *)pc+1))
> +			if (get_user(instr2, tpc + 1))
>  				goto die_sig;
>  			instr2 = __mem_to_opcode_thumb16(instr2);
>  			instr = __opcode_thumb32_compose(instr, instr2);
>  		}
>  	} else {
> -		if (get_user(instr, (u32 __user *)pc))
> +		u32 __user *apc = (u32 __user *)pc;
> +
> +		if (get_user(instr, apc))
>  			goto die_sig;
> +
>  		instr = __mem_to_opcode_arm(instr);
>  	}
>  
> @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>  	}
>  #endif
>  	arm_notify_die("Oops - undefined instruction", regs,
> -		       SIGILL, ILL_ILLOPC, pc, 0, 6);
> +		       SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>  }
>  NOKPROBE_SYMBOL(do_undefinstr)
>  
> 

-- 
Regards,
  Zhen Lei