sparc64: Use arch_validate_flags() to validate ADI flag

When userspace calls mprotect() to enable ADI on an address range,
do_mprotect_pkey() calls arch_validate_prot() to validate new
protection flags. arch_validate_prot() for sparc looks at the first
VMA associated with address range to verify if ADI can indeed be
enabled on this address range. This has two issues - (1) Address
range might cover multiple VMAs while arch_validate_prot() looks at
only the first VMA, (2) arch_validate_prot() peeks at VMA without
holding mmap lock which can result in race condition.

arch_validate_flags() from commit c462ac288f2c ("mm: Introduce
arch_validate_flags()") allows for VMA flags to be validated for all
VMAs that cover the address range given by user while holding mmap
lock. This patch updates sparc code to move the VMA check from
arch_validate_prot() to arch_validate_flags() to fix above two
issues.

Suggested-by: Jann Horn <jannh@google.com>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com>
---
 arch/sparc/include/asm/mman.h | 54 +++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 25 deletions(-)

Hi Khalid,

On Fri, Oct 23, 2020 at 11:56:11AM -0600, Khalid Aziz wrote:
> diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h
> index f94532f25db1..274217e7ed70 100644
> --- a/arch/sparc/include/asm/mman.h
> +++ b/arch/sparc/include/asm/mman.h
> @@ -57,35 +57,39 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr)
>  {
>  	if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_ADI))
>  		return 0;
> -	if (prot & PROT_ADI) {
> -		if (!adi_capable())
> -			return 0;
> +	return 1;
> +}

We kept the equivalent of !adi_capable() check in the arm64
arch_validate_prot() and left arch_validate_flags() more relaxed. I.e.
you can pass PROT_MTE to mmap() even if the hardware doesn't support
MTE. This is in line with the pre-MTE ABI where unknown mmap() flags
would be ignored while mprotect() would reject them. This discrepancy
isn't nice but we decided to preserve the pre-MTE mmap ABI behaviour.
Anyway, it's up to you if you want to change the sparc behaviour, I
don't think it matters in practice.

I think with this patch, arch_validate_prot() no longer needs the 'addr'
argument. Maybe you can submit an additional patch to remove them (not
urgent, the compiler should get rid of them).

>  
> -		if (addr) {
> -			struct vm_area_struct *vma;
> +#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags)
> +/* arch_validate_flags() - Ensure combination of flags is valid for a
> + *	VMA.
> + */
> +static inline bool arch_validate_flags(unsigned long vm_flags)
> +{
> +	/* If ADI is being enabled on this VMA, check for ADI
> +	 * capability on the platform and ensure VMA is suitable
> +	 * for ADI
> +	 */
> +	if (vm_flags & VM_SPARC_ADI) {
> +		if (!adi_capable())
> +			return false;
>  
> -			vma = find_vma(current->mm, addr);
> -			if (vma) {
> -				/* ADI can not be enabled on PFN
> -				 * mapped pages
> -				 */
> -				if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> -					return 0;
> +		/* ADI can not be enabled on PFN mapped pages */
> +		if (vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			return false;
>  
> -				/* Mergeable pages can become unmergeable
> -				 * if ADI is enabled on them even if they
> -				 * have identical data on them. This can be
> -				 * because ADI enabled pages with identical
> -				 * data may still not have identical ADI
> -				 * tags on them. Disallow ADI on mergeable
> -				 * pages.
> -				 */
> -				if (vma->vm_flags & VM_MERGEABLE)
> -					return 0;
> -			}
> -		}
> +		/* Mergeable pages can become unmergeable
> +		 * if ADI is enabled on them even if they
> +		 * have identical data on them. This can be
> +		 * because ADI enabled pages with identical
> +		 * data may still not have identical ADI
> +		 * tags on them. Disallow ADI on mergeable
> +		 * pages.
> +		 */
> +		if (vm_flags & VM_MERGEABLE)
> +			return false;

Ah, you added a check to the madvise(MADV_MERGEABLE) path to ignore the
flag if VM_SPARC_ADI. On arm64 we intercept memcmp_pages() but we have a
PG_arch_2 flag to mark a page as containing tags. Either way should
work.

FWIW, if you are happy with the mmap() rejecting PROT_ADI on
!adi_capable() hardware:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

On 11/20/20 11:01 AM, Catalin Marinas wrote:
> Hi Khalid,
> 
> On Fri, Oct 23, 2020 at 11:56:11AM -0600, Khalid Aziz wrote:
>> diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h
>> index f94532f25db1..274217e7ed70 100644
>> --- a/arch/sparc/include/asm/mman.h
>> +++ b/arch/sparc/include/asm/mman.h
>> @@ -57,35 +57,39 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr)
>>   {
>>   	if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_ADI))
>>   		return 0;
>> -	if (prot & PROT_ADI) {
>> -		if (!adi_capable())
>> -			return 0;
>> +	return 1;
>> +}
> 
> We kept the equivalent of !adi_capable() check in the arm64
> arch_validate_prot() and left arch_validate_flags() more relaxed. I.e.
> you can pass PROT_MTE to mmap() even if the hardware doesn't support
> MTE. This is in line with the pre-MTE ABI where unknown mmap() flags
> would be ignored while mprotect() would reject them. This discrepancy
> isn't nice but we decided to preserve the pre-MTE mmap ABI behaviour.
> Anyway, it's up to you if you want to change the sparc behaviour, I
> don't think it matters in practice.

Hi Catalin,

Thanks for taking a look at this patch. I felt mmap() silently accepting 
PROT_ADI but not really enabling protection can be dangerous since it 
leads the end user to be under false impression that they have protected 
the memory. I chose to treat PROT_ADI as a known flag and provide a 
definite feedback to user whether it can be honored or not.

> 
> I think with this patch, arch_validate_prot() no longer needs the 'addr'
> argument. Maybe you can submit an additional patch to remove them (not
> urgent, the compiler should get rid of them).

Yes, 'addr' is an unused argument now. On the other hand, I suspect with 
additional protections being implemented in hardware for memory regions, 
sooner or later someone will see a need to validate protection bits in 
the context of memory region it is being applied to. Address is not 
going to be enough information though and we are most likely going to 
need size of the memory region being operated upon as well. That means 
this code is likely to need a patch to add the size argument. So it is 
reasonable to remove 'addr' for now and reintroduce a more complete 
version with size as well later in a patch when the need comes up.

> 
>>   
>> -		if (addr) {
>> -			struct vm_area_struct *vma;
>> +#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags)
>> +/* arch_validate_flags() - Ensure combination of flags is valid for a
>> + *	VMA.
>> + */
>> +static inline bool arch_validate_flags(unsigned long vm_flags)
>> +{
>> +	/* If ADI is being enabled on this VMA, check for ADI
>> +	 * capability on the platform and ensure VMA is suitable
>> +	 * for ADI
>> +	 */
>> +	if (vm_flags & VM_SPARC_ADI) {
>> +		if (!adi_capable())
>> +			return false;
>>   
>> -			vma = find_vma(current->mm, addr);
>> -			if (vma) {
>> -				/* ADI can not be enabled on PFN
>> -				 * mapped pages
>> -				 */
>> -				if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>> -					return 0;
>> +		/* ADI can not be enabled on PFN mapped pages */
>> +		if (vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>> +			return false;
>>   
>> -				/* Mergeable pages can become unmergeable
>> -				 * if ADI is enabled on them even if they
>> -				 * have identical data on them. This can be
>> -				 * because ADI enabled pages with identical
>> -				 * data may still not have identical ADI
>> -				 * tags on them. Disallow ADI on mergeable
>> -				 * pages.
>> -				 */
>> -				if (vma->vm_flags & VM_MERGEABLE)
>> -					return 0;
>> -			}
>> -		}
>> +		/* Mergeable pages can become unmergeable
>> +		 * if ADI is enabled on them even if they
>> +		 * have identical data on them. This can be
>> +		 * because ADI enabled pages with identical
>> +		 * data may still not have identical ADI
>> +		 * tags on them. Disallow ADI on mergeable
>> +		 * pages.
>> +		 */
>> +		if (vm_flags & VM_MERGEABLE)
>> +			return false;
> 
> Ah, you added a check to the madvise(MADV_MERGEABLE) path to ignore the
> flag if VM_SPARC_ADI. On arm64 we intercept memcmp_pages() but we have a
> PG_arch_2 flag to mark a page as containing tags. Either way should
> work.
> 
> FWIW, if you are happy with the mmap() rejecting PROT_ADI on
> !adi_capable() hardware:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 

Thanks!

--
Khalid