On 29/03/2019 15:59, Catalin Marinas wrote: > On Tue, Mar 19, 2019 at 03:15:42PM +0000, Vincenzo Frascino wrote: >> Currently, compat tasks running on arm64 can allocate memory up to >> TASK_SIZE_32 (UL(0x100000000)). >> >> This means that mmap() allocations, if we treat them as returning an >> array, are not compliant with the sections 6.5.8 of the C standard >> (C99) which states that: "If the expression P points to an element of >> an array object and the expression Q points to the last element of the >> same array object, the pointer expression Q+1 compares greater than P". >> >> Redefine TASK_SIZE_32 to address the issue. >> >> Cc: Catalin Marinas <catalin.marinas@xxxxxxx> >> Cc: Will Deacon <will.deacon@xxxxxxx> >> Cc: Jann Horn <jannh@xxxxxxxxxx> >> Reported-by: Jann Horn <jannh@xxxxxxxxxx> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@xxxxxxx> >> --- >> arch/arm64/include/asm/processor.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h >> index 07c873fce961..4c689740940d 100644 >> --- a/arch/arm64/include/asm/processor.h >> +++ b/arch/arm64/include/asm/processor.h >> @@ -57,7 +57,7 @@ >> #define TASK_SIZE_64 (UL(1) << vabits_user) >> >> #ifdef CONFIG_COMPAT >> -#define TASK_SIZE_32 UL(0x100000000) >> +#define TASK_SIZE_32 (UL(0x100000000) - PAGE_SIZE) >> #define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \ >> TASK_SIZE_32 : TASK_SIZE_64) >> #define TASK_SIZE_OF(tsk) (test_tsk_thread_flag(tsk, TIF_32BIT) ? \ > > So what I meant with the previous comment, can we have this: > > #ifndef CONFIG_ARM64_64K_PAGES > #define TASK_SIZE_32 (UK(0x100000000) - PAGE_SIZE) > #endif > > and still have a vectors page with 64K configuration? Assuming that it > is not unmapped, an mmap() wouldn't return the 0xffff0000 page to break > the C99 requirements. There is the case of user unmapping the vectors > page (which seems to be allowed based on my reading of the code) and > then getting an mmap() at the end for the 32-bit address range but I > really don't think we should cover for this case. > The current set is designed to cover all the cases, but I am fine either way. > Another option which I think would cover the unmap case as well in all > configurations is to handle the limit in arch_get_mmap_end(). We already > define this to handle mmap limitation on 52-bit user VA but you can make > it handle compat tasks (and probably turn it into a static inline > function). > I like this approach more than what I proposed in the current series, but I think this is not easily back-portable to stable. > The other patches for splitting the vectors page in two are still valid > (to be on par with arm32) but they would not be required for this fix. > Ok, to summarize, this is what I am going to do next: - Send a patch that includes your comments about CONFIG_ARM64_64K_PAGES. - Send a separate series for kuser helpers leaving them enabled by default in all the cases. - Create a new series based on arch_get_mmap_end(). -- Regards, Vincenzo
