| Message ID | 20201023175611.12819-1-khalid.aziz@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sparc64: Use arch_validate_flags() to validate ADI flag | expand |
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
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; +} - 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; } - return 1; + return true; } #endif /* CONFIG_SPARC64 */
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(-)