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

On Tue, Jun 16, 2020 at 07:40:17AM -0700, Dave Hansen wrote:

On 6/16/20 12:49 AM, Kees Cook wrote:

quoted

+	/* 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!

quoted

+	/* 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