The whole static protection magic is silently fixing up anything which is
handed in. That's just wrong. The offending call sites need to be fixed.
Add a debug mechanism which emits a warning if a requested mapping needs to be
fixed up. The DETECT debug mechanism is really not meant to be enabled except
for developers, so limit the output hard to the protection fixups.
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/mm/pageattr.c | 61 +++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 52 insertions(+), 9 deletions(-)
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -42,6 +42,13 @@ struct cpa_data {
struct page **pages;
};
+enum cpa_warn {
+ CPA_PROTECT,
+ CPA_DETECT,
+};
+
+static const int cpa_warn_level = CPA_PROTECT;
+
/*
* Serialize cpa() (for !DEBUG_PAGEALLOC which uses large identity mappings)
* using cpa_lock. So that we don't allow any other cpu, with stale large tlb
@@ -392,6 +399,28 @@ static pgprotval_t protect_kernel_text_r
}
#endif
+static inline bool conflicts(pgprot_t prot, pgprotval_t val)
+{
+ return (pgprot_val(prot) & ~val) != pgprot_val(prot);
+}
+
+static inline void check_conflict(int warnlvl, pgprot_t prot, pgprotval_t val,
+ unsigned long start, unsigned long end,
+ unsigned long pfn, const char *txt)
+{
+ static const char *lvltxt[] = {
+ [CPA_PROTECT] = "protect",
+ [CPA_DETECT] = "detect",
+ };
+
+ if (warnlvl > cpa_warn_level || !conflicts(prot, val))
+ return;
+
+ pr_warn("CPA %8s %10s: 0x%016lx - 0x%016lx PFN %lx req %016llx prevent %016llx\n",
+ lvltxt[warnlvl], txt, start, end, pfn, (unsigned long long)pgprot_val(prot),
+ (unsigned long long)val);
+}
+
/*
* Certain areas of memory on x86 require very specific protection flags,
* for example the BIOS area or kernel text. Callers don't always get this
@@ -399,19 +428,31 @@ static pgprotval_t protect_kernel_text_r
* checks and fixes these known static required protection bits.
*/
static inline pgprot_t static_protections(pgprot_t prot, unsigned long start,
- unsigned long pfn, unsigned long npg)
+ unsigned long pfn, unsigned long npg,
+ int warnlvl)
{
- pgprotval_t forbidden;
+ pgprotval_t forbidden, res;
unsigned long end;
/* Operate on the virtual address */
end = start + npg * PAGE_SIZE - 1;
- forbidden = protect_kernel_text(start, end);
- forbidden |= protect_kernel_text_ro(start, end);
+
+ res = protect_kernel_text(start, end);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "Text NX");
+ forbidden = res;
+
+ res = protect_kernel_text_ro(start, end);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "Text RO");
+ forbidden |= res;
/* Check the PFN directly */
- forbidden |= protect_pci_bios(pfn, pfn + npg - 1);
- forbidden |= protect_rodata(pfn, pfn + npg - 1);
+ res = protect_pci_bios(pfn, pfn + npg - 1);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "PCIBIOS NX");
+ forbidden |= res;
+
+ res = protect_rodata(pfn, pfn + npg - 1);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "Rodata RO");
+ forbidden |= res;
return __pgprot(pgprot_val(prot) & ~forbidden);
}
@@ -683,10 +724,11 @@ static int __should_split_large_page(pte
* in it results in a different pgprot than the first one of the
* requested range. If yes, then the page needs to be split.
*/
- new_prot = static_protections(req_prot, address, pfn, 1);
+ new_prot = static_protections(req_prot, address, pfn, 1, CPA_DETECT);
pfn = old_pfn;
for (i = 0, addr = lpaddr; i < numpages; i++, addr += PAGE_SIZE, pfn++) {
- pgprot_t chk_prot = static_protections(req_prot, addr, pfn, 1);
+ pgprot_t chk_prot = static_protections(req_prot, addr, pfn, 1,
+ CPA_DETECT);
if (pgprot_val(chk_prot) != pgprot_val(new_prot))
return 1;
@@ -1296,7 +1338,8 @@ static int __change_page_attr(struct cpa
pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
- new_prot = static_protections(new_prot, address, pfn, 1);
+ new_prot = static_protections(new_prot, address, pfn, 1,
+ CPA_PROTECT);
new_prot = pgprot_clear_protnone_bits(new_prot);
On 09/17/2018 07:29 AM, Thomas Gleixner wrote:
> The whole static protection magic is silently fixing up anything which is
> handed in. That's just wrong. The offending call sites need to be fixed.
>
> Add a debug mechanism which emits a warning if a requested mapping needs to be
> fixed up. The DETECT debug mechanism is really not meant to be enabled except
> for developers, so limit the output hard to the protection fixups.
...
> +enum cpa_warn {
> + CPA_PROTECT,
> + CPA_DETECT,
> +};
> +
> +static const int cpa_warn_level = CPA_PROTECT;
Even if this is intended for developers only, should we also add some
config option here so things like 0day can still get warnings out of this?
Reviewed-by: Dave Hansen <[email protected]>
On Fri, Sep 21, 2018 at 09:40:36AM -0700, Dave Hansen wrote:
> On 09/17/2018 07:29 AM, Thomas Gleixner wrote:
> > The whole static protection magic is silently fixing up anything which is
> > handed in. That's just wrong. The offending call sites need to be fixed.
> >
> > Add a debug mechanism which emits a warning if a requested mapping needs to be
> > fixed up. The DETECT debug mechanism is really not meant to be enabled except
> > for developers, so limit the output hard to the protection fixups.
> ...
> > +enum cpa_warn {
> > + CPA_PROTECT,
> > + CPA_DETECT,
> > +};
> > +
> > +static const int cpa_warn_level = CPA_PROTECT;
>
> Even if this is intended for developers only, should we also add some
> config option here so things like 0day can still get warnings out of this?
>
> Reviewed-by: Dave Hansen <[email protected]>
OTOH, I really wish there was something like: depends !RANDCONFIG
for some of those things, because I triggered
GCC_PLUGIN_STRUCTLEAK_VERBOSE in a randconfig the other day and thought
everything was busted due to the massive output.
Commit-ID: 4046460b867f8b041c81c26c09d3bcad6d6e814e
Gitweb: https://git.kernel.org/tip/4046460b867f8b041c81c26c09d3bcad6d6e814e
Author: Thomas Gleixner <[email protected]>
AuthorDate: Mon, 17 Sep 2018 16:29:11 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Thu, 27 Sep 2018 20:39:38 +0200
x86/mm/cpa: Add debug mechanism
The whole static protection magic is silently fixing up anything which is
handed in. That's just wrong. The offending call sites need to be fixed.
Add a debug mechanism which emits a warning if a requested mapping needs to be
fixed up. The DETECT debug mechanism is really not meant to be enabled except
for developers, so limit the output hard to the protection fixups.
Signed-off-by: Thomas Gleixner <[email protected]>
Reviewed-by: Dave Hansen <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Bin Yang <[email protected]>
Cc: Mark Gross <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/mm/pageattr.c | 61 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 52 insertions(+), 9 deletions(-)
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 61ff27848452..1f2519055b30 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -42,6 +42,13 @@ struct cpa_data {
struct page **pages;
};
+enum cpa_warn {
+ CPA_PROTECT,
+ CPA_DETECT,
+};
+
+static const int cpa_warn_level = CPA_PROTECT;
+
/*
* Serialize cpa() (for !DEBUG_PAGEALLOC which uses large identity mappings)
* using cpa_lock. So that we don't allow any other cpu, with stale large tlb
@@ -395,6 +402,28 @@ static pgprotval_t protect_kernel_text_ro(unsigned long start,
}
#endif
+static inline bool conflicts(pgprot_t prot, pgprotval_t val)
+{
+ return (pgprot_val(prot) & ~val) != pgprot_val(prot);
+}
+
+static inline void check_conflict(int warnlvl, pgprot_t prot, pgprotval_t val,
+ unsigned long start, unsigned long end,
+ unsigned long pfn, const char *txt)
+{
+ static const char *lvltxt[] = {
+ [CPA_PROTECT] = "protect",
+ [CPA_DETECT] = "detect",
+ };
+
+ if (warnlvl > cpa_warn_level || !conflicts(prot, val))
+ return;
+
+ pr_warn("CPA %8s %10s: 0x%016lx - 0x%016lx PFN %lx req %016llx prevent %016llx\n",
+ lvltxt[warnlvl], txt, start, end, pfn, (unsigned long long)pgprot_val(prot),
+ (unsigned long long)val);
+}
+
/*
* Certain areas of memory on x86 require very specific protection flags,
* for example the BIOS area or kernel text. Callers don't always get this
@@ -402,19 +431,31 @@ static pgprotval_t protect_kernel_text_ro(unsigned long start,
* checks and fixes these known static required protection bits.
*/
static inline pgprot_t static_protections(pgprot_t prot, unsigned long start,
- unsigned long pfn, unsigned long npg)
+ unsigned long pfn, unsigned long npg,
+ int warnlvl)
{
- pgprotval_t forbidden;
+ pgprotval_t forbidden, res;
unsigned long end;
/* Operate on the virtual address */
end = start + npg * PAGE_SIZE - 1;
- forbidden = protect_kernel_text(start, end);
- forbidden |= protect_kernel_text_ro(start, end);
+
+ res = protect_kernel_text(start, end);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "Text NX");
+ forbidden = res;
+
+ res = protect_kernel_text_ro(start, end);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "Text RO");
+ forbidden |= res;
/* Check the PFN directly */
- forbidden |= protect_pci_bios(pfn, pfn + npg - 1);
- forbidden |= protect_rodata(pfn, pfn + npg - 1);
+ res = protect_pci_bios(pfn, pfn + npg - 1);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "PCIBIOS NX");
+ forbidden |= res;
+
+ res = protect_rodata(pfn, pfn + npg - 1);
+ check_conflict(warnlvl, prot, res, start, end, pfn, "Rodata RO");
+ forbidden |= res;
return __pgprot(pgprot_val(prot) & ~forbidden);
}
@@ -686,10 +727,11 @@ static int __should_split_large_page(pte_t *kpte, unsigned long address,
* in it results in a different pgprot than the first one of the
* requested range. If yes, then the page needs to be split.
*/
- new_prot = static_protections(req_prot, address, pfn, 1);
+ new_prot = static_protections(req_prot, address, pfn, 1, CPA_DETECT);
pfn = old_pfn;
for (i = 0, addr = lpaddr; i < numpages; i++, addr += PAGE_SIZE, pfn++) {
- pgprot_t chk_prot = static_protections(req_prot, addr, pfn, 1);
+ pgprot_t chk_prot = static_protections(req_prot, addr, pfn, 1,
+ CPA_DETECT);
if (pgprot_val(chk_prot) != pgprot_val(new_prot))
return 1;
@@ -1299,7 +1341,8 @@ repeat:
pgprot_val(new_prot) &= ~pgprot_val(cpa->mask_clr);
pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
- new_prot = static_protections(new_prot, address, pfn, 1);
+ new_prot = static_protections(new_prot, address, pfn, 1,
+ CPA_PROTECT);
new_prot = pgprot_clear_protnone_bits(new_prot);
Hello,
The patch "x86/mm/cpa: Add debug mechanism" (https://lore.kernel.org/lkml/[email protected]/) caused a thousands of messages during boot on my machine.
They look like below:
kernel: CPA protect Text RO: 0xffffffff8affc000 - 0xffffffff8affcfff PFN 12cffc req 0000000000000063 prevent 0000000000000002
kernel: CPA protect Text RO: 0xffffffff8affd000 - 0xffffffff8affdfff PFN 12cffd req 0000000000000063 prevent 0000000000000002
kernel: CPA protect Text RO: 0xffffffff8affe000 - 0xffffffff8affefff PFN 12cffe req 0000000000000063 prevent 0000000000000002
kernel: CPA protect Text RO: 0xffffffff8afff000 - 0xffffffff8affffff PFN 12cfff req 0000000000000063 prevent 0000000000000002
kernel: CPA conflict Rodata RO: 0xffffa2fbece00000 - 0xffffa2fbecffffff PFN 12ce00 req 80000000000000e3 prevent 0000000000000002
kernel: CPA protect Rodata RO: 0xffffa2fbece00000 - 0xffffa2fbece00fff PFN 12ce00 req 8000000000000063 prevent 0000000000000002
kernel: CPA protect Rodata RO: 0xffffa2fbece01000 - 0xffffa2fbece01fff PFN 12ce01 req 8000000000000063 prevent 0000000000000002
kernel: CPA protect Rodata RO: 0xffffa2fbece02000 - 0xffffa2fbece02fff PFN 12ce02 req 8000000000000063 prevent 0000000000000002
The exact reason for those may be related to some out-of-tree patches which I use:
https://github.com/anthraxx/linux-hardened/commit/c492687efa253c32972115a5e110e51614af445e#diff-e1f7e9a10ed7feb8c32493438cff8456
https://github.com/anthraxx/linux-hardened/commit/266d16fc47257b2548e6fcbfafebcc5d84d46389#diff-e1f7e9a10ed7feb8c32493438cff8456
but I wonder if those messages shouldn't be rate-limited or hidden behind 'CONFIG_CPA_DEBUG' in general as there may be a chance that someone will trigger something like that with vanillia kernel.
Jordan