Re: [PATCH 4/8] seccomp: Implement constant action bitmaps

From: Kees Cook <keescook@chromium.org>
To: Dave Hansen <dave.hansen@intel.com>
Cc: linux-kernel@vger.kernel.org,
	Christian Brauner <christian@brauner.io>,
	Sargun Dhillon <sargun@sargun.me>,
	Tycho Andersen <tycho@tycho.ws>, Jann Horn <jannh@google.com>,
	"zhujianwei (C)" <zhujianwei7@huawei.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Matthew Wilcox <willy@infradead.org>,
	Andy Lutomirski <luto@kernel.org>, Will Drewry <wad@chromium.org>,
	Shuah Khan <shuah@kernel.org>, Matt Denton <mpdenton@google.com>,
	Chris Palmer <palmer@google.com>,
	Jeffrey Vander Stoep <jeffv@google.com>,
	Aleksa Sarai <cyphar@cyphar.com>,
	Hehuazhen <hehuazhen@huawei.com>,
	x86@kernel.org,
	Linux Containers <containers@lists.linux-foundation.org>,
	linux-security-module@vger.kernel.org, linux-api@vger.kernel.org
Subject: Re: [PATCH 4/8] seccomp: Implement constant action bitmaps
Date: Tue, 16 Jun 2020 09:01:46 -0700	[thread overview]
Message-ID: <202006160851.E8F9928AAB@keescook> (raw)
In-Reply-To: <fc0c14cd-bcf0-c94c-6cba-d0ce1844e93c@intel.com>

On Tue, Jun 16, 2020 at 07:40:17AM -0700, Dave Hansen wrote:
> On 6/16/20 12:49 AM, Kees Cook wrote:
> > +	/* Mark the second page as untouched (i.e. "old") */
> > +	preempt_disable();
> > +	set_pte_at(&init_mm, vaddr, ptep, pte_mkold(*(READ_ONCE(ptep))));
> > +	local_flush_tlb_kernel_range(vaddr, vaddr + PAGE_SIZE);
> > +	preempt_enable();
> 
> If you can, I'd wrap that nugget up in a helper.  I'd also suggest being
> very explicit in a comment about what it is trying to do: ensure no TLB
> entries exist so that a future access will always set the Accessed bit.

Yeah, good idea!

> 
> > +	/* Make sure the PTE agrees that it is untouched. */
> > +	if (WARN_ON_ONCE(sd_touched(ptep)))
> > +		return;
> > +	/* Read a portion of struct seccomp_data from the second page. */
> > +	check = sd->instruction_pointer;
> > +	/* First, verify the contents are zero from vzalloc(). */
> > +	if (WARN_ON_ONCE(check))
> > +		return;
> > +	/* Now make sure the ACCESSED bit has been set after the read. */
> > +	if (!sd_touched(ptep)) {
> > +		/*
> > +		 * If autodetection fails, fall back to standard beahavior by
> > +		 * clearing the entire "allow" bitmap.
> > +		 */
> > +		pr_warn_once("seccomp: cannot build automatic syscall filters\n");
> > +		bitmap_zero(bitmaps->allow, NR_syscalls);
> > +		return;
> > +	}
> 
> I can't find any big holes with this.  It's the kind of code that makes
> me nervous, but mostly because it's pretty different that anything else
> we have in the kernel.
> 
> It's also clear to me here that you probably have a slightly different
> expectation of what the PTE accessed flag means versus the hardware
> guys.  What you are looking for it to mean is roughly: "a retired
> instruction touched this page".
> 
> The hardware guys would probably say it's closer to "a TLB entry was
> established for this page."  Remember that TLB entries can be
> established speculatively or from things like prefetchers.  While I
> don't know of anything microarchitectural today which would trip this
> mechanism, it's entirely possible that something in the future might.
> Accessing close to the page boundary is the exact kind of place folks
> might want to optimize.

Yeah, and to that end, going the cBPF emulator route removes this kind
of "weird" behavior.

> 
> *But*, at least it would err in the direction of being conservative.  It
> would say "somebody touched the page!" more often than it should, but
> never _less_ often than it should.

Right -- I made sure to design the bitmaps and the direction of the
checking to fail towards running the filter instead of bypassing it.

> One thing about the implementation (which is roughly):
> 
> 	// Touch the data:
> 	check = sd->instruction_pointer;
> 	// Examine the PTE mapping that data:
> 	if (!sd_touched(ptep)) {
> 		// something
> 	}
> 
> There aren't any barriers in there, which could lead to the sd_touched()
> check being ordered before the data touch.  I think a rmb() will
> suffice.  You could even do it inside sd_touched().

Ah yeah, I had convinced myself that READ_ONCE() gained me that
coverage, but I guess that's not actually true here.

> Was there a reason you chose to export a ranged TLB flush?  I probably
> would have just used the single-page flush_tlb_one_kernel() for this
> purpose if I were working in arch-specific code.

No particular reason -- it just seemed easiest to make available given
the interfaces. I could do the single-page version instead, if this way
of doing things survives review. ;)

Thanks for looking at it!

-- 
Kees Cook