Re: [PATCH v2 0/6] Use generic code for randomization of virtual address of x86

[Xiongwei Song <sxwjean@me.com>]

Hi Experts,

Gentle reminder.  Any comments would be appreciated.
Thank you so much.

Regards,
Xiongwei

> On Oct 11, 2021, at 10:31 PM, sxwjean@me.com wrote:
> 
> From: Xiongwei Song <sxwjean@gmail.com>
> 
> Hello,
> 
> This patchset are to use generic code for randomization of virtual address
> of x86. Since the basic code logic of x86 is same as generic code, so no
> need to implement these functions on x86.
> 
> Patch 1~3 are prepared to change the generic code to apply to x86.
> 
> Patch 4 is to switch to generic arch_pick_mmap_layout() with 
> ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT enabled. Also provided basically
> test and the result was put in commit message too.
> 
> Patch 5~6 are used to handle the legacy things.
> 
> Test programs(to verify if the entropy of return value of mmap is kept
> after applying the patchset):
> - C code for mmap test:
> 	#include <stdio.h>
> 	#include <stdlib.h>
> 	#include <sys/mman.h>
> 
> 	int main(int argc, char *argv[])
> 	{
> 		unsigned long *addr;
> 
> 		addr = mmap(NULL, 4096, PROT_READ, MAP_SHARED|MAP_ANONYMOUS, -1, 0);
> 		if (addr == MAP_FAILED) {
> 			printf("NULL\n");
> 		} else {
> 			printf("%lx\n", (unsigned long)addr);
> 			munmap(addr, 4096);
> 		}
> 
> 		return 0;
> 	}
> 
> - Shell script for collecting output of C progarm above and give a
>  statistics:
> 	#!/bin/bash
> 	declare -a COUNT
> 
> 	if [ "$1" == "" ]; then
> 	    echo "Please give a test number!"
> 	    exit 1
> 	fi
> 	number=$1
> 
> 	for ((i=0; i<$number; i++))
> 	do
> 	    addr=$(mmaptest)
> 	    addr=$(((16#$addr&0xf000000000)>>36))
> 	    COUNT[$addr]=$((COUNT[$addr]+1))
> 	done
> 
> 	echo "    Virtual Address Range     |   hit times   "
> 	echo "----------------------------------------"
> 	for ((i=0; i<16; i++))
> 	do
> 	    j=`echo "obase=16; $i" | bc`
> 	    echo "0x7f${j,,}000000000 - 0x7f${j,,}ffffff000 |   ${COUNT[i]}"
> 	done
> 
> Run 10 thousands times C progam, collect the output with shell script, get
> the test results below:
> 	Before the patchset:
>            Virtual Address Range       | hit times
>        ----------------------------------------
>        0x7f0000000000 - 0x7f0ffffff000 |   655 
>        0x7f1000000000 - 0x7f1ffffff000 |   617 
>        0x7f2000000000 - 0x7f2ffffff000 |   636 
>        0x7f3000000000 - 0x7f3ffffff000 |   625 
>        0x7f4000000000 - 0x7f4ffffff000 |   651 
>        0x7f5000000000 - 0x7f5ffffff000 |   591 
>        0x7f6000000000 - 0x7f6ffffff000 |   623 
>        0x7f7000000000 - 0x7f7ffffff000 |   627 
>        0x7f8000000000 - 0x7f8ffffff000 |   638 
>        0x7f9000000000 - 0x7f9ffffff000 |   586 
>        0x7fa000000000 - 0x7faffffff000 |   637 
>        0x7fb000000000 - 0x7fbffffff000 |   607 
>        0x7fc000000000 - 0x7fcffffff000 |   618 
>        0x7fd000000000 - 0x7fdffffff000 |   656 
>        0x7fe000000000 - 0x7feffffff000 |   614 
>        0x7ff000000000 - 0x7ffffffff000 |   619 
> 
> 	After the patchset:
>            Virtual Address Range       | hit times
>        ----------------------------------------
>        0x7f0000000000 - 0x7f0ffffff000 |   661 
>        0x7f1000000000 - 0x7f1ffffff000 |   645 
>        0x7f2000000000 - 0x7f2ffffff000 |   609 
>        0x7f3000000000 - 0x7f3ffffff000 |   594 
>        0x7f4000000000 - 0x7f4ffffff000 |   616 
>        0x7f5000000000 - 0x7f5ffffff000 |   622 
>        0x7f6000000000 - 0x7f6ffffff000 |   617 
>        0x7f7000000000 - 0x7f7ffffff000 |   582 
>        0x7f8000000000 - 0x7f8ffffff000 |   618 
>        0x7f9000000000 - 0x7f9ffffff000 |   629 
>        0x7fa000000000 - 0x7faffffff000 |   635 
>        0x7fb000000000 - 0x7fbffffff000 |   625 
>        0x7fc000000000 - 0x7fcffffff000 |   614 
>        0x7fd000000000 - 0x7fdffffff000 |   610 
>        0x7fe000000000 - 0x7feffffff000 |   648
>        0x7ff000000000 - 0x7ffffffff000 |   675
> 
> v1 -> v2:
> - Spilt the patch 2 of v1 as Kees suggested.
> - Drop patch 1 of v1, which renamed TIF_ADDR32 to TIF_32BIT, which is
>  unreasonable for x86. Because in x86, 64bit process can call 32bit
>  syscall. Thanks Peterz for pointing this out. 
> 
> v1:
> - https://lkml.org/lkml/2021/9/21/482
> - https://lkml.org/lkml/2021/9/21/484
> - https://lkml.org/lkml/2021/9/27/688
> 
> Please review.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Gabriel Krisman Bertazi <krisman@collabora.com>
> Cc: Lai Jiangshan <laijs@linux.alibaba.com>
> Cc: Huang Rui <ray.huang@amd.com>
> Cc: Yazen Ghannam <yazen.ghannam@amd.com>
> Cc: Kim Phillips <kim.phillips@amd.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Balbir Singh <sblbir@amazon.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: sxwjean@me.com
> Cc: linux-kernel@vger.kernel.org
> 
> Xiongwei Song (6):
>  mm/util: Assign a meaningful value to mmap_legacy_base
>  mm/util: Allow to pass a specific task size when getting mmapping base
>  mm/util: Support CONFIG_HAVE_ARCH_COMPAT_MMAP_BASES
>  x86/mm: Randomize VA with generit arch_pick_mmap_layout()
>  x86/mm: Discard the defination of HAVE_ARCH_PICK_MMAP_LAYOUT
>  x86/elf: Discard ARCH_HAS_ELF_RANDOMIZE selection
> 
> arch/x86/Kconfig                 |   2 +-
> arch/x86/include/asm/compat.h    |   5 ++
> arch/x86/include/asm/processor.h |   5 +-
> arch/x86/mm/mmap.c               | 112 -------------------------------
> mm/util.c                        |  35 +++++++---
> 5 files changed, 37 insertions(+), 122 deletions(-)
> 
> -- 
> 2.30.2
>