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

[Dave Hansen <dave.hansen@intel.com>]

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.

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

*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.

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().

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.