lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 16 Jun 2020 07:40:17 -0700 From: Dave Hansen <dave.hansen@...el.com> To: Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org Cc: Christian Brauner <christian@...uner.io>, Sargun Dhillon <sargun@...gun.me>, Tycho Andersen <tycho@...ho.ws>, Jann Horn <jannh@...gle.com>, "zhujianwei (C)" <zhujianwei7@...wei.com>, Dave Hansen <dave.hansen@...ux.intel.com>, Matthew Wilcox <willy@...radead.org>, Andy Lutomirski <luto@...nel.org>, Will Drewry <wad@...omium.org>, Shuah Khan <shuah@...nel.org>, Matt Denton <mpdenton@...gle.com>, Chris Palmer <palmer@...gle.com>, Jeffrey Vander Stoep <jeffv@...gle.com>, Aleksa Sarai <cyphar@...har.com>, Hehuazhen <hehuazhen@...wei.com>, x86@...nel.org, Linux Containers <containers@...ts.linux-foundation.org>, linux-security-module@...r.kernel.org, linux-api@...r.kernel.org Subject: Re: [PATCH 4/8] seccomp: Implement constant action bitmaps 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.
Powered by blists - more mailing lists