Re: [PATCH] Revert "mm/usercopy: Drop extra is_vmalloc_or_module() check"

From: Kefeng Wang <wangkefeng.wang@huawei.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
	Kees Cook <keescook@chromium.org>,
	Laura Abbott <labbott@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH] Revert "mm/usercopy: Drop extra is_vmalloc_or_module() check"
Date: Fri, 24 Dec 2021 15:06:25 +0800	[thread overview]
Message-ID: <96fe1826-aeaf-4ea0-9f01-03d6b3933b34@huawei.com> (raw)
In-Reply-To: <ffd77bcf-b9d8-956c-9f83-14b9f0b496e7@csgroup.eu>


On 2021/12/24 14:01, Christophe Leroy wrote:
>
> Le 23/12/2021 à 11:21, Kefeng Wang a écrit :
>> This reverts commit 517e1fbeb65f5eade8d14f46ac365db6c75aea9b.
>>
>>     usercopy: Kernel memory exposure attempt detected from SLUB object not in SLUB page?! (offset 0, size 1048)!
>>     kernel BUG at mm/usercopy.c:99
>>     ...
>>     usercopy_abort+0x64/0xa0 (unreliable)
>>     __check_heap_object+0x168/0x190
>>     __check_object_size+0x1a0/0x200
>>     dev_ethtool+0x2494/0x2b20
>>     dev_ioctl+0x5d0/0x770
>>     sock_do_ioctl+0xf0/0x1d0
>>     sock_ioctl+0x3ec/0x5a0
>>     __se_sys_ioctl+0xf0/0x160
>>     system_call_exception+0xfc/0x1f0
>>     system_call_common+0xf8/0x200
>>
>> When run ethtool eth0, the BUG occurred, the code shows below,
>>
>>     data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
>>     copy_to_user(useraddr, data, gstrings.len * ETH_GSTRING_LEN))
>>
>> The data is alloced by vmalloc(),  virt_addr_valid(ptr) will return true
>> on PowerPC64, which leads to the panic, add back the is_vmalloc_or_module()
>> check to fix it.
> Is it expected that virt_addr_valid() returns true on PPC64 for
> vmalloc'ed memory ? If that's the case it also means that
> CONFIG_DEBUG_VIRTUAL won't work as expected either.

Our product reports this bug to me, after let them do some test,

I found virt_addr_valid return true for vmalloc'ed memory on their board.

I think DEBUG_VIRTUAL could not be work well too, but I can't test it.

>
> If it is unexpected, I think you should fix PPC64 instead of adding this
> hack back. Maybe the ARM64 fix can be used as a starting point, see
> commit 68dd8ef32162 ("arm64: memory: Fix virt_addr_valid() using
> __is_lm_address()")

Yes， I check the history,  fix virt_addr_valid() on PowerPC is what I 
firstly want to do,

but I am not familiar with PPC, and also HARDENED_USERCOPY on other's 
ARCHs could

has this issue too, so I add the workaround back.


1) PPC maintainer/expert, any suggestion ?

2) Maybe we could add some check to WARN this scenario.

--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -229,6 +229,8 @@ static inline void check_heap_object(const void 
*ptr, unsigned long n,
         if (!virt_addr_valid(ptr))
                 return;

+       WARN_ON_ONCE(is_vmalloc_or_module_addr(ptr));

> In the meantime, can you provide more information on your config,
> especially which memory model is used ?

Some useful configs,

CONFIG_PPC64=y
CONFIG_PPC_BOOK3E_64=y
CONFIG_E5500_CPU=y
CONFIG_TARGET_CPU_BOOL=y
CONFIG_PPC_BOOK3E=y
CONFIG_E500=y
CONFIG_PPC_E500MC=y
CONFIG_PPC_FPU=y
CONFIG_FSL_EMB_PERFMON=y
CONFIG_FSL_EMB_PERF_EVENT=y
CONFIG_FSL_EMB_PERF_EVENT_E500=y
CONFIG_BOOKE=y
CONFIG_PPC_FSL_BOOK3E=y
CONFIG_PTE_64BIT=y
CONFIG_PHYS_64BIT=y
CONFIG_PPC_MMU_NOHASH=y
CONFIG_PPC_BOOK3E_MMU=y
CONFIG_SELECT_MEMORY_MODEL=y
CONFIG_FLATMEM_MANUAL=y
CONFIG_FLATMEM=y
CONFIG_FLAT_NODE_MEM_MAP=y
CONFIG_SPARSEMEM_VMEMMAP_ENABLE=y

>
> Christophe
