Re: [PATCH v2] x86: consider effective protection attributes in W+X check
>>> On 23.02.18 at 08:49, wrote:
> * Jan Beulich wrote:
>
>> >>> On 21.02.18 at 17:53, wrote:
>>
>> > * Jan Beulich wrote:
>> >
>> >> Using just the leaf page table entry flags would cause a false warning
>> >> in case _PAGE_RW is clear or _PAGE_NX is set in a higher level entry.
>> >> Hand through both the current entry's flags as well as the accumulated
>> >> effective value (the latter as pgprotval_t instead of pgprot_t, as it's
>> >> not an actual entry's value).
>> >>
>> >> This in particular eliminates the false W+X warning when running under
>> >> Xen, as commit 2cc42bac1c ("x86-64/Xen: eliminate W+X mappings") has to
>> >> make the necessary adjustment in L2 rather than L1 (the reason is
>> >> explained there). I.e. _PAGE_RW is clear there in L1, but _PAGE_NX is
>> >> set in L2.
>> >>
>> >> Signed-off-by: Jan Beulich
>> >> Reviewed-by: Juergen Gross
>> >> ---
>> >> v2: Re-base onto tip tree. Add Xen related paragraph to description.
>> >> ---
>> >> arch/x86/mm/dump_pagetables.c | 92
>> > ++
>> >> 1 file changed, 57 insertions(+), 35 deletions(-)
>> >
>> > There's a build failure with CONFIG_KASAN=y enabled:
>> >
>> > arch/x86/mm/dump_pagetables.c: In function ‘kasan_page_table’:
>> > arch/x86/mm/dump_pagetables.c:365:3: error: too few arguments to function
>> > ‘note_page’
>> > arch/x86/mm/dump_pagetables.c:238:13: note: declared here
>>
>> Oh, I see. Question though is what to pass as the extra argument:
>> Do I need to pass in the caller's effective rights, or should I take
>> kasan_page_table()'s checking against kasan_zero_p?d as an
>> indication that the effective permission is zero here? I'm sorry for
>> this probably trivial question, but I know nothing about how KASAN
>> works.
>
> I'm not sure either - but I've Cc:-ed the KASAN gents who might be able to
> help us out here?
Actually, the "zero" in the names of the symbols meanwhile makes
me be pretty sure passing 0 for the effective permissions here is
exactly what is wanted. I'm about to produce v3.
Jan
Re: [PATCH v2] x86: consider effective protection attributes in W+X check
* Jan Beulich wrote:
> >>> On 21.02.18 at 17:53, wrote:
>
> > * Jan Beulich wrote:
> >
> >> Using just the leaf page table entry flags would cause a false warning
> >> in case _PAGE_RW is clear or _PAGE_NX is set in a higher level entry.
> >> Hand through both the current entry's flags as well as the accumulated
> >> effective value (the latter as pgprotval_t instead of pgprot_t, as it's
> >> not an actual entry's value).
> >>
> >> This in particular eliminates the false W+X warning when running under
> >> Xen, as commit 2cc42bac1c ("x86-64/Xen: eliminate W+X mappings") has to
> >> make the necessary adjustment in L2 rather than L1 (the reason is
> >> explained there). I.e. _PAGE_RW is clear there in L1, but _PAGE_NX is
> >> set in L2.
> >>
> >> Signed-off-by: Jan Beulich
> >> Reviewed-by: Juergen Gross
> >> ---
> >> v2: Re-base onto tip tree. Add Xen related paragraph to description.
> >> ---
> >> arch/x86/mm/dump_pagetables.c | 92
> > ++
> >> 1 file changed, 57 insertions(+), 35 deletions(-)
> >
> > There's a build failure with CONFIG_KASAN=y enabled:
> >
> > arch/x86/mm/dump_pagetables.c: In function ‘kasan_page_table’:
> > arch/x86/mm/dump_pagetables.c:365:3: error: too few arguments to function
> > ‘note_page’
> > arch/x86/mm/dump_pagetables.c:238:13: note: declared here
>
> Oh, I see. Question though is what to pass as the extra argument:
> Do I need to pass in the caller's effective rights, or should I take
> kasan_page_table()'s checking against kasan_zero_p?d as an
> indication that the effective permission is zero here? I'm sorry for
> this probably trivial question, but I know nothing about how KASAN
> works.
I'm not sure either - but I've Cc:-ed the KASAN gents who might be able to
help us out here?
Thanks,
Ingo
Re: [PATCH v2] x86: consider effective protection attributes in W+X check
>>> On 21.02.18 at 17:53, wrote:
> * Jan Beulich wrote:
>
>> Using just the leaf page table entry flags would cause a false warning
>> in case _PAGE_RW is clear or _PAGE_NX is set in a higher level entry.
>> Hand through both the current entry's flags as well as the accumulated
>> effective value (the latter as pgprotval_t instead of pgprot_t, as it's
>> not an actual entry's value).
>>
>> This in particular eliminates the false W+X warning when running under
>> Xen, as commit 2cc42bac1c ("x86-64/Xen: eliminate W+X mappings") has to
>> make the necessary adjustment in L2 rather than L1 (the reason is
>> explained there). I.e. _PAGE_RW is clear there in L1, but _PAGE_NX is
>> set in L2.
>>
>> Signed-off-by: Jan Beulich
>> Reviewed-by: Juergen Gross
>> ---
>> v2: Re-base onto tip tree. Add Xen related paragraph to description.
>> ---
>> arch/x86/mm/dump_pagetables.c | 92
> ++
>> 1 file changed, 57 insertions(+), 35 deletions(-)
>
> There's a build failure with CONFIG_KASAN=y enabled:
>
> arch/x86/mm/dump_pagetables.c: In function ‘kasan_page_table’:
> arch/x86/mm/dump_pagetables.c:365:3: error: too few arguments to function
> ‘note_page’
> arch/x86/mm/dump_pagetables.c:238:13: note: declared here
Oh, I see. Question though is what to pass as the extra argument:
Do I need to pass in the caller's effective rights, or should I take
kasan_page_table()'s checking against kasan_zero_p?d as an
indication that the effective permission is zero here? I'm sorry for
this probably trivial question, but I know nothing about how KASAN
works.
Jan
Re: [PATCH v2] x86: consider effective protection attributes in W+X check
* Jan Beulich wrote:
> Using just the leaf page table entry flags would cause a false warning
> in case _PAGE_RW is clear or _PAGE_NX is set in a higher level entry.
> Hand through both the current entry's flags as well as the accumulated
> effective value (the latter as pgprotval_t instead of pgprot_t, as it's
> not an actual entry's value).
>
> This in particular eliminates the false W+X warning when running under
> Xen, as commit 2cc42bac1c ("x86-64/Xen: eliminate W+X mappings") has to
> make the necessary adjustment in L2 rather than L1 (the reason is
> explained there). I.e. _PAGE_RW is clear there in L1, but _PAGE_NX is
> set in L2.
>
> Signed-off-by: Jan Beulich
> Reviewed-by: Juergen Gross
> ---
> v2: Re-base onto tip tree. Add Xen related paragraph to description.
> ---
> arch/x86/mm/dump_pagetables.c | 92
> ++
> 1 file changed, 57 insertions(+), 35 deletions(-)
There's a build failure with CONFIG_KASAN=y enabled:
arch/x86/mm/dump_pagetables.c: In function ‘kasan_page_table’:
arch/x86/mm/dump_pagetables.c:365:3: error: too few arguments to function
‘note_page’
arch/x86/mm/dump_pagetables.c:238:13: note: declared here
Thanks,
Ingo
[PATCH v2] x86: consider effective protection attributes in W+X check
Using just the leaf page table entry flags would cause a false warning
in case _PAGE_RW is clear or _PAGE_NX is set in a higher level entry.
Hand through both the current entry's flags as well as the accumulated
effective value (the latter as pgprotval_t instead of pgprot_t, as it's
not an actual entry's value).
This in particular eliminates the false W+X warning when running under
Xen, as commit 2cc42bac1c ("x86-64/Xen: eliminate W+X mappings") has to
make the necessary adjustment in L2 rather than L1 (the reason is
explained there). I.e. _PAGE_RW is clear there in L1, but _PAGE_NX is
set in L2.
Signed-off-by: Jan Beulich
Reviewed-by: Juergen Gross
---
v2: Re-base onto tip tree. Add Xen related paragraph to description.
---
arch/x86/mm/dump_pagetables.c | 92 ++
1 file changed, 57 insertions(+), 35 deletions(-)
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -29,6 +29,7 @@
struct pg_state {
int level;
pgprot_t current_prot;
+ pgprotval_t effective_prot;
unsigned long start_address;
unsigned long current_address;
const struct addr_marker *marker;
@@ -235,9 +236,9 @@ static unsigned long normalize_addr(unsi
* print what we collected so far.
*/
static void note_page(struct seq_file *m, struct pg_state *st,
- pgprot_t new_prot, int level)
+ pgprot_t new_prot, pgprotval_t new_eff, int level)
{
- pgprotval_t prot, cur;
+ pgprotval_t prot, cur, eff;
static const char units[] = "BKMGTPE";
/*
@@ -247,23 +248,24 @@ static void note_page(struct seq_file *m
*/
prot = pgprot_val(new_prot);
cur = pgprot_val(st->current_prot);
+ eff = st->effective_prot;
if (!st->level) {
/* First entry */
st->current_prot = new_prot;
+ st->effective_prot = new_eff;
st->level = level;
st->marker = address_markers;
st->lines = 0;
pt_dump_seq_printf(m, st->to_dmesg, "---[ %s ]---\n",
st->marker->name);
- } else if (prot != cur || level != st->level ||
+ } else if (prot != cur || new_eff != eff || level != st->level ||
st->current_address >= st->marker[1].start_address) {
const char *unit = units;
unsigned long delta;
int width = sizeof(unsigned long) * 2;
- pgprotval_t pr = pgprot_val(st->current_prot);
- if (st->check_wx && (pr & _PAGE_RW) && !(pr & _PAGE_NX)) {
+ if (st->check_wx && (eff & _PAGE_RW) && !(eff & _PAGE_NX)) {
WARN_ONCE(1,
"x86/mm: Found insecure W+X mapping at
address %p/%pS\n",
(void *)st->start_address,
@@ -317,21 +319,30 @@ static void note_page(struct seq_file *m
st->start_address = st->current_address;
st->current_prot = new_prot;
+ st->effective_prot = new_eff;
st->level = level;
}
}
-static void walk_pte_level(struct seq_file *m, struct pg_state *st, pmd_t
addr, unsigned long P)
+static inline pgprotval_t effective_prot(pgprotval_t prot1, pgprotval_t prot2)
+{
+ return (prot1 & prot2 & (_PAGE_USER | _PAGE_RW)) |
+ ((prot1 | prot2) & _PAGE_NX);
+}
+
+static void walk_pte_level(struct seq_file *m, struct pg_state *st, pmd_t addr,
+ pgprotval_t eff_in, unsigned long P)
{
int i;
pte_t *start;
- pgprotval_t prot;
+ pgprotval_t prot, eff;
start = (pte_t *)pmd_page_vaddr(addr);
for (i = 0; i < PTRS_PER_PTE; i++) {
prot = pte_flags(*start);
+ eff = effective_prot(eff_in, prot);
st->current_address = normalize_addr(P + i * PTE_LEVEL_MULT);
- note_page(m, st, __pgprot(prot), 5);
+ note_page(m, st, __pgprot(prot), eff, 5);
start++;
}
}
@@ -366,42 +377,45 @@ static inline bool kasan_page_table(stru
#if PTRS_PER_PMD > 1
-static void walk_pmd_level(struct seq_file *m, struct pg_state *st, pud_t
addr, unsigned long P)
+static void walk_pmd_level(struct seq_file *m, struct pg_state *st, pud_t addr,
+ pgprotval_t eff_in, unsigned long P)
{
int i;
pmd_t *start, *pmd_start;
- pgprotval_t prot;
+ pgprotval_t prot, eff;
pmd_start = start = (pmd_t *)pud_page_vaddr(addr);
for (i = 0; i < PTRS_PER_PMD; i++) {
st->current_address = normalize_addr(P + i * PMD_LEVEL_MULT);
if (!pmd_none(*start)) {
+ prot = pmd_flags(*start);
+ eff = effective_prot(eff_in, prot);
if (pmd_large(*start) || !pmd_present(*start)) {
-
