Re: [PATCH 1/6] syscalls: use uaccess_kernel in addr_limit_user_check

From: Guenter Roeck <linux@roeck-us.net>
To: Christoph Hellwig <hch@lst.de>
Cc: Nick Hu <nickhu@andestech.com>, Greentime Hu <green.hu@gmail.com>,
	Vincent Chen <deanbo422@gmail.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-riscv@lists.infradead.org, linux-arch@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/6] syscalls: use uaccess_kernel in addr_limit_user_check
Date: Mon, 20 Jul 2020 22:30:30 -0700	[thread overview]
Message-ID: <7fc565fe-411e-6a0b-8aaf-0bf808f0d6a9@roeck-us.net> (raw)
In-Reply-To: <20200721052022.GA10011@lst.de>

On 7/20/20 10:20 PM, Christoph Hellwig wrote:
> On Mon, Jul 20, 2020 at 10:15:37PM -0700, Guenter Roeck wrote:
>>>> -       if (CHECK_DATA_CORRUPTION(uaccess_kernel(),
>>>> +       if (CHECK_DATA_CORRUPTION(!uaccess_kernel(),
>>>>
>>>> How does this work anywhere ?
>>>
>>> No, that is the wrong check - we want to make sure the address
>>> space override doesn't leak to userspace.  The problem is that
>>> armnommu (and m68knommu, but that doesn't call the offending
>>> function) pretends to not have a kernel address space, which doesn't
>>> really work.  Here is the fix I sent out yesterday, which I should
>>> have Cc'ed you on, sorry:
>>>
>>
>> The patch below makes sense, and it does work, but I still suspect
>> that something with your original patch is wrong, or at least suspicious.
>> Reason: My change above (Adding the "!") works for _all_ of my arm boot
>> tests. Or, in other words, it doesn't make a difference if true
>> or false is passed as first parameter of CHECK_DATA_CORRUPTION(), except
>> for nommu systems. Also, unless I am really missing something, your
>> original patch _does_ reverse the logic.
> 
> Well.  segment_eq is in current mainline used in two places:
> 
>  1) to implement uaccess_kernel
>  2) in addr_limit_user_check to implement uaccess_kernel-like
>     semantics using a strange reverse notation
> 
> I think the explanation for your observation is how addr_limit_user_check
> is called on arm.  The addr_limit_check_failed wrapper for it is called
> from assembly code, but only after already checking the addr_limit,
> basically duplicating the segment_eq check.  So for mmu builds it won't
> get called unless we leak the kernel address space override, which
> is a pretty fatal error and won't show up in your boot tests.  The
> only good way to test it is by explicit injecting it using the
> lkdtm module.
> 

Guess I lost it somewhere. Are you saying the check was wrong all along
and your patch fixed it ?

Thanks,
Guenter