linux-kernel - Re: [PATCH v2 03/15] powerpc/align: Convert emulate_spe() to user_access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cc59942e-7f8c-196d-a734-07674178e189@csgroup.eu>
Date:   Thu, 11 Mar 2021 06:45:39 +0100
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Daniel Axtens <dja@...ens.net>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>
Cc:     linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 03/15] powerpc/align: Convert emulate_spe() to
 user_access_begin



Le 10/03/2021 à 23:31, Daniel Axtens a écrit :
> Hi Christophe,
> 
>> This patch converts emulate_spe() to using user_access_being
> s/being/begin/ :)
>> logic.
>>
>> Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
>> pagefault_disable()"), might_fault() doesn't fire when called from
>> sections where pagefaults are disabled, which must be the case
>> when using _inatomic variants of __get_user and __put_user. So
>> the might_fault() in user_access_begin() is not a problem.
> (likewise with the might_fault() in __get_user_nocheck, called from
> unsafe_get_user())

unsafe_get_user() call __get_user_nocheck() with do_allow = false, so there is no might_fault() there.

> 
>> There was a verification of user_mode() together with the access_ok(),
>> but the function returns in case !user_mode() immediately after
>> the access_ok() verification, so removing that test has no effect.
> 
> I agree that removing the test is safe.
> 
>> -	/* Verify the address of the operand */
>> -	if (unlikely(user_mode(regs) &&
>> -		     !access_ok(addr, nb)))
>> -		return -EFAULT;
>> -
> 
> I found the reasoning a bit confusing: I think it's safe to remove
> because:

Ok, I'll see if I can rephrase it.

> 
>   - we have the usermode check immediately following it:
> 
>>   	/* userland only */
>>   	if (unlikely(!user_mode(regs)))
>>   		return 0;
> 
>   - and then we have the access_ok() check as part of
>     user_read_access_begin later on in the function:
> 
>> +		if (!user_read_access_begin(addr, nb))
>> +			return -EFAULT;
>> +
> 
> 
>>   		switch (nb) {
>>   		case 8:
>> -			ret |= __get_user_inatomic(temp.v[0], p++);
>> -			ret |= __get_user_inatomic(temp.v[1], p++);
>> -			ret |= __get_user_inatomic(temp.v[2], p++);
>> -			ret |= __get_user_inatomic(temp.v[3], p++);
>> +			unsafe_get_user(temp.v[0], p++, Efault_read);
>> +			unsafe_get_user(temp.v[1], p++, Efault_read);
>> +			unsafe_get_user(temp.v[2], p++, Efault_read);
>> +			unsafe_get_user(temp.v[3], p++, Efault_read);
> 
> This will bail early rather than trying every possible read. I think
> that's OK. 

It tries every possible read, but at the end it bails out with EFAULT, so I see no point.

> I can't think of a situation where we could fail to read the
> first byte and then successfully read later bytes, for example. Also I
> can't think of a sane way userspace could depend on that behaviour. So I
> agree with this change (and the change to the write path).
> 
>>   			fallthrough;
>>   		case 4:
>> -			ret |= __get_user_inatomic(temp.v[4], p++);
>> -			ret |= __get_user_inatomic(temp.v[5], p++);
>> +			unsafe_get_user(temp.v[4], p++, Efault_read);
>> +			unsafe_get_user(temp.v[5], p++, Efault_read);
>>   			fallthrough;
>>   		case 2:
>> -			ret |= __get_user_inatomic(temp.v[6], p++);
>> -			ret |= __get_user_inatomic(temp.v[7], p++);
>> -			if (unlikely(ret))
>> -				return -EFAULT;
>> +			unsafe_get_user(temp.v[6], p++, Efault_read);
>> +			unsafe_get_user(temp.v[7], p++, Efault_read);
>>   		}
>> +		user_read_access_end();
>>   
>>   		switch (instr) {
>>   		case EVLDD:
>> @@ -255,31 +250,41 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>>   
>>   	/* Store result to memory or update registers */
>>   	if (flags & ST) {
>> -		ret = 0;
>>   		p = addr;
>> +
>> +		if (!user_read_access_begin(addr, nb))
> 
> That should be a user_write_access_begin.

Good catch thanks.

> 
>> +			return -EFAULT;
>> +
> 
> 
>>   
>>   	return 1;
>> +
>> +Efault_read:
> 
> Checkpatch complains that this is CamelCase, which seems like a
> checkpatch problem. Efault_{read,write} seem like good labels to me.

I'm not keen of names mixing capital letters and lowercase, but Efault is the label that has been 
used almost everywhere with unsafe_get/put_user(), so I inclined myself.

> 
> (You don't need to change anything, I just like to check the checkpatch
> results when reviewing a patch.)
> 
>> +	user_read_access_end();
>> +	return -EFAULT;
>> +
>> +Efault_write:
>> +	user_write_access_end();
>> +	return -EFAULT;
>>   }
>>   #endif /* CONFIG_SPE */
>>
> 
> With the user_write_access_begin change:
>    Reviewed-by: Daniel Axtens <dja@...ens.net>
> 
> Kind regards,
> Daniel
>