Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290909 > unrolled thread

4.4-rc5: ugly warn on: 5 W+X pages found

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-14 09:10 +0100
    Re: 4.4-rc5: ugly warn on: 5 W+X pages found Borislav Petkov <bp@alien8.de> - 2015-12-14 10:00 +0100
      Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-14 10:10 +0100
        Re: 4.4-rc5: ugly warn on: 5 W+X pages found Borislav Petkov <bp@alien8.de> - 2015-12-14 10:20 +0100
        Re: 4.4-rc5: ugly warn on: 5 W+X pages found Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-14 20:20 +0100
          Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-14 21:30 +0100
            Re: 4.4-rc5: ugly warn on: 5 W+X pages found Andy Lutomirski <luto@amacapital.net> - 2015-12-14 22:10 +0100
              Re: 4.4-rc5: ugly warn on: 5 W+X pages found Arjan van de Ven <arjan@linux.intel.com> - 2015-12-14 22:30 +0100
                Re: 4.4-rc5: ugly warn on: 5 W+X pages found Andy Lutomirski <luto@amacapital.net> - 2015-12-14 23:30 +0100
                  Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 10:50 +0100
                    Re: 4.4-rc5: ugly warn on: 5 W+X pages found Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-15 18:50 +0100
                      Re: 4.4-rc5: ugly warn on: 5 W+X pages found Borislav Petkov <bp@alien8.de> - 2015-12-15 19:40 +0100
                        Re: 4.4-rc5: ugly warn on: 5 W+X pages found Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-15 20:10 +0100
                          Re: 4.4-rc5: ugly warn on: 5 W+X pages found Borislav Petkov <bp@alien8.de> - 2015-12-15 20:20 +0100
                      Re: 4.4-rc5: ugly warn on: 5 W+X pages found Andy Lutomirski <luto@amacapital.net> - 2015-12-15 19:50 +0100
                        Re: 4.4-rc5: ugly warn on: 5 W+X pages found Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-15 20:10 +0100
                      Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 22:00 +0100
                        4.4.-rc5: lguest causes ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 22:20 +0100
                          Re: 4.4.-rc5: lguest causes ugly warn on: 5 W+X pages found Rusty Russell <rusty@rustcorp.com.au> - 2015-12-16 03:30 +0100
                            Re: 4.4.-rc5: lguest causes ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-16 09:20 +0100
                        Re: 4.4-rc5: ugly warn on: 5 W+X pages found Borislav Petkov <bp@alien8.de> - 2015-12-15 22:40 +0100
                          Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 23:10 +0100
                            Re: 4.4-rc5: ugly warn on: 5 W+X pages found Borislav Petkov <bp@alien8.de> - 2015-12-15 23:20 +0100
                Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 09:00 +0100
                  [PATCH 1/2] x86_32/mm: Set NX in __supported_pte_mask before enabling paging Andy Lutomirski <luto@kernel.org> - 2015-12-15 09:10 +0100
                  [PATCH 2/2] x86/mm: Make kmap_prot into a #define Andy Lutomirski <luto@kernel.org> - 2015-12-15 09:10 +0100
                  [PATCH 0/2] x86/mm: A _PAGE_NX fixlet and a kmap cleanup Andy Lutomirski <luto@kernel.org> - 2015-12-15 09:10 +0100
                  Re: 4.4-rc5: ugly warn on: 5 W+X pages found Arjan van de Ven <arjan@linux.intel.com> - 2015-12-15 14:40 +0100
                    Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 15:10 +0100
                      Re: 4.4-rc5: ugly warn on: 5 W+X pages found "H. Peter Anvin" <hpa@zytor.com> - 2015-12-15 17:30 +0100
                        Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-15 18:50 +0100
    Re: 4.4-rc5: ugly warn on: 5 W+X pages found Pavel Machek <pavel@ucw.cz> - 2015-12-14 13:40 +0100

Page 1 of 2  [1] 2  Next page →

#1290909 — 4.4-rc5: ugly warn on: 5 W+X pages found

Hi!

> Kernel complains:

And now, we are at -rc5, and kernel still complains...

...with a back trace, which is clearly completely useless, and just
there to make it scary and make people report.

Problem is... noone cares for the reports. (-rc0 version below).

Can we get rid of that WARN_ON? If you do care about reports, please
add email address those can be reported to. If you don't... just drop
it.

Hardware is thinkpad x60.

								Pavel

[    3.267542] NX-protecting the kernel data: 5764k
[    3.278325] ------------[ cut here ]------------
[    3.282060] WARNING: CPU: 1 PID: 1 at
arch/x86/mm/dump_pagetables.c:225 note_page+0x5ec/0x790()
[    3.285993] x86/mm: Found insecure W+X mapping at address
ffe69000/0xffe69000
[    3.289991] Modules linked in:
[    3.293995] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0-rc5+
#132
[    3.298178] Hardware name: LENOVO 17097HU/17097HU, BIOS 7BETD8WW
(2.19 ) 03/31/2011
[    3.304343]  00000001 00000000 f5cffeac c42ba698 f5cffed8 f5cffec8
c404059b 000000e1
[    3.308840]  c403cb9c f5cfff50 00000163 00000000 f5cffee0 c40405f6
00000009 f5cffed8
[    3.313419]  c4d3347c f5cffef4 f5cfff1c c403cb9c c4d2c08c 000000e1
c4d3347c ffe69000
[    3.317935] Call Trace:
[    3.322395]  [<c42ba698>] dump_stack+0x41/0x59
[    3.326787]  [<c404059b>] warn_slowpath_common+0x6b/0xa0
[    3.331171]  [<c403cb9c>] ? note_page+0x5ec/0x790
[    3.335522]  [<c40405f6>] warn_slowpath_fmt+0x26/0x30
[    3.339839]  [<c403cb9c>] note_page+0x5ec/0x790
[    3.344154]  [<c403ce9f>] ptdump_walk_pgd_level_core+0x15f/0x240
[    3.348440]  [<c403cfa1>] ptdump_walk_pgd_level_checkwx+0x11/0x20
[    3.352611]  [<c4034fdd>] mark_rodata_ro+0xcd/0xf0
[    3.356677]  [<c4a55c97>] kernel_init+0x17/0xc0
[    3.360702]  [<c4a5c409>] ret_from_kernel_thread+0x21/0x38
[    3.364739]  [<c4a55c80>] ? rest_init+0xa0/0xa0
[    3.368709] ---[ end trace 7121849c40f4a5ba ]---
[    3.372716] x86/mm: Checked W+X mappings: FAILED, 5 W+X pages
found.


> [    5.256044] ------------[ cut here ]------------
> [    5.259267] WARNING: CPU: 0 PID: 1 at
> arch/x86/mm/dump_pagetables.c:225 note_page+0x5ec/0x790()
> [    5.262668] x86/mm: Found insecure W+X mapping at address
> ffe69000/0xffe69000
> [    5.267109] Modules linked in:
> [    5.271403] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.3.0+ #122
> [    5.275679] Hardware name: LENOVO 17097HU/17097HU, BIOS 7BETD8WW
> (2.19 ) 03/31/2011
> [    5.279957]  00000000 00000000 f5cffeac c42b9f18 f5cffed8 f5cffec8
> c404062b 000000e1
> [    5.284387]  c403ca9c f5cfff50 00000163 00000000 f5cffee0 c4040686
> 00000009 f5cffed8
> [    5.288815]  c4d268ac f5cffef4 f5cfff1c c403ca9c c4d1f494 000000e1
> c4d268ac ffe69000
> [    5.293314] Call Trace:
> [    5.297602]  [<c42b9f18>] dump_stack+0x41/0x59
> [    5.301864]  [<c404062b>] warn_slowpath_common+0x6b/0xa0
> [    5.306054]  [<c403ca9c>] ? note_page+0x5ec/0x790
> [    5.310209]  [<c4040686>] warn_slowpath_fmt+0x26/0x30
> [    5.314358]  [<c403ca9c>] note_page+0x5ec/0x790
> [    5.318440]  [<c403cd8f>] ptdump_walk_pgd_level_core+0x14f/0x230
> [    5.322578]  [<c403ce91>] ptdump_walk_pgd_level_checkwx+0x11/0x20
> [    5.326632]  [<c4034ead>] mark_rodata_ro+0xcd/0xf0
> [    5.330625]  [<c4a4aab7>] kernel_init+0x17/0xc0
> [    5.334585]  [<c4a511c9>] ret_from_kernel_thread+0x21/0x38
> [    5.338585]  [<c4a4aaa0>] ? rest_init+0xa0/0xa0
> [    5.342583] ---[ end trace bc9ac0874ad9a058 ]---
> [    5.346630] x86/mm: Checked W+X mappings: FAILED, 5 W+X pages
> found.
> 
> ...I'm not quite sure why it does backtrace, or how to debug this
> one...
> 
> 									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1290986

On Mon, Dec 14, 2015 at 09:04:03AM +0100, Pavel Machek wrote:
> Hi!
> 
> > Kernel complains:
> 
> And now, we are at -rc5, and kernel still complains...

You can disable CONFIG_DEBUG_WX in your .config:

54727e6e950a ("x86: don't make DEBUG_WX default to 'y' even with DEBUG_RODATA")

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290988

On Mon 2015-12-14 09:58:03, Borislav Petkov wrote:
> On Mon, Dec 14, 2015 at 09:04:03AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > Kernel complains:
> > 
> > And now, we are at -rc5, and kernel still complains...
> 
> You can disable CONFIG_DEBUG_WX in your .config:
> 
> 54727e6e950a ("x86: don't make DEBUG_WX default to 'y' even with DEBUG_RODATA")

I know. But either someone cares, and it should be fixes, or noone
cares, and the check should be removed.

Backtrace should be removed in any case, as it is useless.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1290993

On Mon, Dec 14, 2015 at 10:07:26AM +0100, Pavel Machek wrote:
> I know. But either someone cares, and it should be fixes, or noone
> cares, and the check should be removed.

Someone cares.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291496

On Mon, Dec 14, 2015 at 1:07 AM, Pavel Machek <pavel@ucw.cz> wrote:
>
> I know. But either someone cares, and it should be fixes, or noone
> cares, and the check should be removed.

Someone cares, and it should be scheduled to be fixed for 4.5. The EFI
mapping changes that were required to avoid the warning were much too
big and late to make 4.4.

So for now, don't enable CONFIG_DEBUG_WX for now. Unless you want to
actively debug the EFI mapping changes, that is. Which I heartily
recommend people doing.

(The patches are in Matt Fleming's tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git efi-pgtable

and I think they are also merged in the -tip tree for next, but I
haven't double-checked)

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291531

Hi!

> > I know. But either someone cares, and it should be fixes, or noone
> > cares, and the check should be removed.
> 
> Someone cares, and it should be scheduled to be fixed for 4.5. The EFI
> mapping changes that were required to avoid the warning were much too
> big and late to make 4.4.
> 
> So for now, don't enable CONFIG_DEBUG_WX for now. Unless you want to
> actively debug the EFI mapping changes, that is. Which I heartily
> recommend people doing.

Ok, good, except... This is thinkpad X60. Good old BIOS. It should
have no EFI.

pavel@duo:~$ dmesg | grep EFI
pavel@duo:~$

From the messages I got:

> [    3.285993] x86/mm: Found insecure W+X mapping at address
> ffe69000/0xffe69000

---[ Persisent kmap() Area ]---
0xffc00000-0xffd28000        1184K                               pte
0xffd28000-0xffddd000         724K     RW                 GLB NX pte
0xffddd000-0xffe69000         560K                               pte
0xffe69000-0xffe6e000          20K     RW                 GLB x  pte
0xffe6e000-0xffe6f000           4K                               pte
---[ Fixmap Area ]---

That is not EFI, right?

Thanks,
									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291560

On Mon, Dec 14, 2015 at 12:26 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > I know. But either someone cares, and it should be fixes, or noone
>> > cares, and the check should be removed.
>>
>> Someone cares, and it should be scheduled to be fixed for 4.5. The EFI
>> mapping changes that were required to avoid the warning were much too
>> big and late to make 4.4.
>>
>> So for now, don't enable CONFIG_DEBUG_WX for now. Unless you want to
>> actively debug the EFI mapping changes, that is. Which I heartily
>> recommend people doing.
>
> Ok, good, except... This is thinkpad X60. Good old BIOS. It should
> have no EFI.
>
> pavel@duo:~$ dmesg | grep EFI
> pavel@duo:~$
>
> From the messages I got:
>
>> [    3.285993] x86/mm: Found insecure W+X mapping at address
>> ffe69000/0xffe69000
>
> ---[ Persisent kmap() Area ]---
> 0xffc00000-0xffd28000        1184K                               pte
> 0xffd28000-0xffddd000         724K     RW                 GLB NX pte
> 0xffddd000-0xffe69000         560K                               pte
> 0xffe69000-0xffe6e000          20K     RW                 GLB x  pte
> 0xffe6e000-0xffe6f000           4K                               pte
> ---[ Fixmap Area ]---
>
> That is not EFI, right?

That's weird.  The only API to do that seems to be manually setting
kmap_prot to _PAGE_KERNEL_EXEC, and nothing does that.  (Why is
kmap_prot a variable on x86 at all?  It has exactly one writer, and
that's the code that initializes it in the first place.  Shouldn't we
#define kmap_prot _PAGE_KERNEL?

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291588

> That's weird.  The only API to do that seems to be manually setting
> kmap_prot to _PAGE_KERNEL_EXEC, and nothing does that.  (Why is
> kmap_prot a variable on x86 at all?  It has exactly one writer, and
> that's the code that initializes it in the first place.  Shouldn't we
> #define kmap_prot _PAGE_KERNEL?

iirc it changes based on runtime detection of NX capability

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291609

On Mon, Dec 14, 2015 at 1:24 PM, Arjan van de Ven <arjan@linux.intel.com> wrote:
>
>> That's weird.  The only API to do that seems to be manually setting
>> kmap_prot to _PAGE_KERNEL_EXEC, and nothing does that.  (Why is
>> kmap_prot a variable on x86 at all?  It has exactly one writer, and
>> that's the code that initializes it in the first place.  Shouldn't we
>> #define kmap_prot _PAGE_KERNEL?
>
>
> iirc it changes based on runtime detection of NX capability
>

Maybe it did, but if it still does, I can't find the code.

What *does* change is __supported_pte_mask.  If we're willing to make
disable_nx work a little less well, we could try to initialize
__supported_pte_mask from the very beginning.  (We currently seem to
detect and enable NX even before we enable paging.)  I suspect that
Pavel is seeing a kmap mapping left over from so early that it didn't
have NX set (killed by massage_pgprot).

Borislav, could we do that?  (Why do we have disable_nx at all?  I
suspect it was for debugging a long, long time ago.)

Alternatively, we could go through and set NX everywhere after we
decide we have NX, but that seems rather error-prone.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1291984

On Mon 2015-12-14 14:25:10, Andy Lutomirski wrote:
> On Mon, Dec 14, 2015 at 1:24 PM, Arjan van de Ven <arjan@linux.intel.com> wrote:
> >
> >> That's weird.  The only API to do that seems to be manually setting
> >> kmap_prot to _PAGE_KERNEL_EXEC, and nothing does that.  (Why is
> >> kmap_prot a variable on x86 at all?  It has exactly one writer, and
> >> that's the code that initializes it in the first place.  Shouldn't we
> >> #define kmap_prot _PAGE_KERNEL?
> >
> >
> > iirc it changes based on runtime detection of NX capability
> >
> 
> Maybe it did, but if it still does, I can't find the code.
> 
> What *does* change is __supported_pte_mask.  If we're willing to make
> disable_nx work a little less well, we could try to initialize
> __supported_pte_mask from the very beginning.  (We currently seem to
> detect and enable NX even before we enable paging.)  I suspect that
> Pavel is seeing a kmap mapping left over from so early that it didn't
> have NX set (killed by massage_pgprot).

I tried applying:

[PATCH 1/2] x86_32/mm: Set NX in __supported_pte_mask before enabling
paging

but I still get

[    2.685402] ------------[ cut here ]------------
[    2.688649] WARNING: CPU: 0 PID: 1 at
arch/x86/mm/dump_pagetables.c:225 note_
page+0x5ec/0x790()
[    2.691897] x86/mm: Found insecure W+X mapping at address
ffe69000/0xffe69000
[    2.695090] Modules linked in:

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292410

On Tue, Dec 15, 2015 at 1:40 AM, Pavel Machek <pavel@ucw.cz> wrote:
>
> I tried applying:
>
> [PATCH 1/2] x86_32/mm: Set NX in __supported_pte_mask before enabling
> paging
>
> but I still get
>
> [    2.691897] x86/mm: Found insecure W+X mapping at address ffe69000/0xffe69000

This may be an insane suggestion, but how about we try to detect when
that entry gets set, rather than after the fact.

Something really brute-force like

  diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
  index 6ec0c8b2e9df..538c9bb239b9 100644
  --- a/arch/x86/include/asm/pgtable.h
  +++ b/arch/x86/include/asm/pgtable.h
  @@ -337,6 +337,13 @@ static inline pmd_t pmd_clear_soft_dirty(pmd_t pmd)

   #endif /* CONFIG_HAVE_ARCH_SOFT_DIRTY */

  +static inline int kernel_write_execute_prot(pgprotval_t protval)
  +{
  +       return !(protval & _PAGE_USER) &&
  +               !(protval & _PAGE_NX) &&
  +               (protval & _PAGE_RW);
  +}
  +
   /*
    * Mask out unsupported bits in a present pgprot.  Non-present pgprots
    * can use those bits for other purposes, so leave them be.
  @@ -345,8 +352,10 @@ static inline pgprotval_t massage_pgprot(pgprot_t pgprot)
   {
          pgprotval_t protval = pgprot_val(pgprot);

  -       if (protval & _PAGE_PRESENT)
  +       if (protval & _PAGE_PRESENT) {
                  protval &= __supported_pte_mask;
  +               WARN_ON_ONCE(kernel_write_execute_prot(protval));
  +       }

          return protval;
   }

or similar?

The above is entirely untested. Maybe it doesn't compile. Or boot. Or work.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292450

On Tue, Dec 15, 2015 at 09:45:40AM -0800, Linus Torvalds wrote:
> On Tue, Dec 15, 2015 at 1:40 AM, Pavel Machek <pavel@ucw.cz> wrote:
> >
> > I tried applying:
> >
> > [PATCH 1/2] x86_32/mm: Set NX in __supported_pte_mask before enabling
> > paging
> >
> > but I still get
> >
> > [    2.691897] x86/mm: Found insecure W+X mapping at address ffe69000/0xffe69000
> 
> This may be an insane suggestion, but how about we try to detect when
> that entry gets set, rather than after the fact.
> 
> Something really brute-force like
> 
>   diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
>   index 6ec0c8b2e9df..538c9bb239b9 100644
>   --- a/arch/x86/include/asm/pgtable.h
>   +++ b/arch/x86/include/asm/pgtable.h
>   @@ -337,6 +337,13 @@ static inline pmd_t pmd_clear_soft_dirty(pmd_t pmd)
> 
>    #endif /* CONFIG_HAVE_ARCH_SOFT_DIRTY */
> 
>   +static inline int kernel_write_execute_prot(pgprotval_t protval)
>   +{
>   +       return !(protval & _PAGE_USER) &&
>   +               !(protval & _PAGE_NX) &&

Shouldn't this be without a "!"? AFAIU, we want _PAGE_NX | _PAGE_RW?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292477

On Tue, Dec 15, 2015 at 10:30 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Dec 15, 2015 at 09:45:40AM -0800, Linus Torvalds wrote:
>>
>>   +static inline int kernel_write_execute_prot(pgprotval_t protval)
>>   +{
>>   +       return !(protval & _PAGE_USER) &&
>>   +               !(protval & _PAGE_NX) &&
>
> Shouldn't this be without a "!"? AFAIU, we want _PAGE_NX | _PAGE_RW?

We want to *warn* about PAGE_RW without PAGE_NX.  No?

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292482

On Tue, Dec 15, 2015 at 11:06:38AM -0800, Linus Torvalds wrote:
> We want to *warn* about PAGE_RW without PAGE_NX.  No?

Ah yes, we do:

note_page:

	...

	if (st->check_wx && (pr & _PAGE_RW) && !(pr & _PAGE_NX)) {

I got confused, sorry.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292464

On Tue, Dec 15, 2015 at 9:45 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Dec 15, 2015 at 1:40 AM, Pavel Machek <pavel@ucw.cz> wrote:
>>
>> I tried applying:
>>
>> [PATCH 1/2] x86_32/mm: Set NX in __supported_pte_mask before enabling
>> paging
>>
>> but I still get
>>
>> [    2.691897] x86/mm: Found insecure W+X mapping at address ffe69000/0xffe69000
>
> This may be an insane suggestion, but how about we try to detect when
> that entry gets set, rather than after the fact.
>
> Something really brute-force like
>
>   diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
>   index 6ec0c8b2e9df..538c9bb239b9 100644
>   --- a/arch/x86/include/asm/pgtable.h
>   +++ b/arch/x86/include/asm/pgtable.h
>   @@ -337,6 +337,13 @@ static inline pmd_t pmd_clear_soft_dirty(pmd_t pmd)
>
>    #endif /* CONFIG_HAVE_ARCH_SOFT_DIRTY */
>
>   +static inline int kernel_write_execute_prot(pgprotval_t protval)
>   +{
>   +       return !(protval & _PAGE_USER) &&
>   +               !(protval & _PAGE_NX) &&
>   +               (protval & _PAGE_RW);
>   +}
>   +
>    /*
>     * Mask out unsupported bits in a present pgprot.  Non-present pgprots
>     * can use those bits for other purposes, so leave them be.
>   @@ -345,8 +352,10 @@ static inline pgprotval_t massage_pgprot(pgprot_t pgprot)
>    {
>           pgprotval_t protval = pgprot_val(pgprot);
>
>   -       if (protval & _PAGE_PRESENT)
>   +       if (protval & _PAGE_PRESENT) {
>                   protval &= __supported_pte_mask;
>   +               WARN_ON_ONCE(kernel_write_execute_prot(protval));

Shouldn't we switch those two lines?  Arguably trying to set rwx
permissions on !PAE is a bug even if it makes no difference, whereas
setting rw- and getting rwx because we don't have NX support on the
platform or kernel build doesn't indicate a bug.

Anyway, I still think that we should apply my patches for 4.5 because
I think they're cleanups, but apparently I guessed wrong as to what
was causing Pavel's issue.  But your patch would help diagnose it.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292478

On Tue, Dec 15, 2015 at 10:40 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Shouldn't we switch those two lines?

No. This is for debugging Pavel's issue. If somehow the
__supported_pte_mask ends up having NX clear again (memory scribble,
whatever), we very explicitly would want to catch that.

I'm *not* suggesting this as a generic patch to be applied anywhere
else. That patch is meant to debug Pavel's particular setup, and
nothing else.

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292543

Hi!

> > I tried applying:
> >
> > [PATCH 1/2] x86_32/mm: Set NX in __supported_pte_mask before enabling
> > paging
> >
> > but I still get
> >
> > [    2.691897] x86/mm: Found insecure W+X mapping at address ffe69000/0xffe69000
> 
> This may be an insane suggestion, but how about we try to detect when
> that entry gets set, rather than after the fact.
> 
> Something really brute-force like
> 
>   diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
>   index 6ec0c8b2e9df..538c9bb239b9 100644
>   --- a/arch/x86/include/asm/pgtable.h
>   +++ b/arch/x86/include/asm/pgtable.h
>   @@ -337,6 +337,13 @@ static inline pmd_t pmd_clear_soft_dirty(pmd_t pmd)
> 
>    #endif /* CONFIG_HAVE_ARCH_SOFT_DIRTY */
> 
>   +static inline int kernel_write_execute_prot(pgprotval_t protval)
>   +{
>   +       return !(protval & _PAGE_USER) &&
>   +               !(protval & _PAGE_NX) &&
>   +               (protval & _PAGE_RW);
>   +}
...
>   +       if (protval & _PAGE_PRESENT) {
>                   protval &= __supported_pte_mask;
>   +               WARN_ON_ONCE(kernel_write_execute_prot(protval));
>   +       }
> 
>           return protval;
>    }
> 
> or similar?
> 
> The above is entirely untested. Maybe it doesn't compile. Or
> boot. Or work.

Well, with two extra spaces at each line, it does not apply :-).

I applied it by hand, and the output is:

[    0.000000] MTRR variable ranges enabled:
[    0.000000]   0 base 000000000 mask F80000000 write-back
[    0.000000]   1 base 080000000 mask FC0000000 write-back
[    0.000000]   2 base 0BF700000 mask FFFF00000 uncachable
[    0.000000]   3 base 0BF800000 mask FFF800000 uncachable
[    0.000000]   4 disabled
[    0.000000]   5 disabled
[    0.000000]   6 disabled
[    0.000000]   7 disabled
[    0.000000] x86/PAT: PAT not supported by CPU.
[    0.000000] initial memory mapped: [mem 0x00000000-0x05bfffff]
[    0.000000] Base memory trampoline at [c009b000] 9b000 size 16384
[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at
./arch/x86/include/asm/pgtable.h:357 kernel_physical_mapping_init+0x
256/0x395()
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.0-rc5+ #137
[    0.000000] Hardware name: LENOVO 17097HU/17097HU, BIOS 7BETD8WW
(2.19 ) 03/31/2011
[    0.000000]  00000000 00000000 c4e63e90 c42baaf8 00000000 c4e63eac
c404066b 00000165
[    0.000000]  c4f134da 00000000 00000000 00000000 c4e63ebc c404070f
00000009 00000000
[    0.000000]  c4e63f18 c4f134da c4e63f00 00000000 00000000 00000000
00000000 00000000
[    0.000000] Call Trace:
[    0.000000]  [<c42baaf8>] dump_stack+0x41/0x59
[    0.000000]  [<c404066b>] warn_slowpath_common+0x6b/0xa0
[    0.000000]  [<c4f134da>] ?
kernel_physical_mapping_init+0x256/0x395
[    0.000000]  [<c404070f>] warn_slowpath_null+0xf/0x20
[    0.000000]  [<c4f134da>] kernel_physical_mapping_init+0x256/0x395
[    0.000000]  [<c4a4de21>] init_memory_mapping+0x191/0x300
[    0.000000]  [<c4f12d96>] init_mem_mapping+0xe7/0x1f3
[    0.000000]  [<c4f12d96>] ? init_mem_mapping+0xe7/0x1f3
[    0.000000]  [<c4f065ef>] setup_arch+0x659/0x8ca
[    0.000000]  [<c4f0480e>] start_kernel+0xbb/0x360
[    0.000000]  [<c4f042d4>] i386_start_kernel+0x82/0x86
[    0.000000] ---[ end trace e117245cd61feaf1 ]---
[    0.000000] BRK [0x0566a000, 0x0566afff] PGTABLE
[    0.000000] BRK [0x0566b000, 0x0566bfff] PGTABLE
[    0.000000] BRK [0x0566c000, 0x0566cfff] PGTABLE

I'll take a look if I can figure out what it means...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292550 — 4.4.-rc5: lguest causes ugly warn on: 5 W+X pages found

Hi!

> > or similar?
> > 
> > The above is entirely untested. Maybe it doesn't compile. Or
> > boot. Or work.
> 
> Well, with two extra spaces at each line, it does not apply :-).
> 
> I applied it by hand, and the output is:
> 
> [    0.000000] MTRR variable ranges enabled:
...> [    0.000000] BRK [0x0566c000, 0x0566cfff] PGTABLE
> 
> I'll take a look if I can figure out what it means...

Wait, there's more in the log.

[    1.952146] Bluetooth: HCI UART protocol H4 registered
[    1.954335] Bluetooth: HCI UART protocol BCSP registered
[    1.956750] usbcore: registered new interface driver btusb
[    1.958953] ------------[ cut here ]------------
[    1.961149] WARNING: CPU: 1 PID: 1 at
./arch/x86/include/asm/pgtable.h:357
vmap_page_range_noflush+0x1f0/0x280()
[    1.963511] Modules linked in:
[    1.965849] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W
4.4.0-rc5+ #137
[    1.968230] Hardware name: LENOVO 17097HU/17097HU, BIOS 7BETD8WW
(2.19 ) 03/31/2011
[    1.970593]  00000001 00000000 f5cffe64 c42baaf8 00000000 f5cffe80
c404066b 00000165
[    1.973103]  c40fbe70 00000163 00000000 00000000 f5cffe90 c404070f
00000009 00000000
[    1.975670]  f5cffee0 c40fbe70 c4f88348 00000000 ffe6dfff ffe6e000
c4f8a018 ffe6dfff
[    1.978304] Call Trace:
[    1.980882]  [<c42baaf8>] dump_stack+0x41/0x59
[    1.983464]  [<c404066b>] warn_slowpath_common+0x6b/0xa0
[    1.986053]  [<c40fbe70>] ? vmap_page_range_noflush+0x1f0/0x280
[    1.988625]  [<c404070f>] warn_slowpath_null+0xf/0x20
[    1.991154]  [<c40fbe70>] vmap_page_range_noflush+0x1f0/0x280
[    1.993676]  [<c40fbf2b>] map_vm_area+0x2b/0x40
[    1.996153]  [<c4f2c795>] init+0xf8/0x1a4
[    1.998591]  [<c4f2c69d>] ? edac_init+0x67/0x67
[    2.001014]  [<c4000442>] do_one_initcall+0xc2/0x1c0
[    2.003391]  [<c4f044e3>] ? initcall_blacklist+0x97/0x97
[    2.005815]  [<c4f044e3>] ? initcall_blacklist+0x97/0x97
[    2.008161]  [<c4051546>] ?
__usermodehelper_set_disable_depth+0x36/0x40
[    2.010518]  [<c407d4a6>] ? up_write+0x16/0x40
[    2.012817]  [<c4f04ba3>] kernel_init_freeable+0xf0/0x16d
[    2.015078]  [<c4f04ba3>] ? kernel_init_freeable+0xf0/0x16d
[    2.017386]  [<c4a4d9c8>] kernel_init+0x8/0xc0
[    2.019661]  [<c4a54149>] ret_from_kernel_thread+0x21/0x38
[    2.021932]  [<c4a4d9c0>] ? rest_init+0xa0/0xa0
[    2.024168] ---[ end trace e117245cd61feaf2 ]---
[    2.026383] lguest: mapped switcher at ffe69000
[    2.028958] sdhci: Secure Digital Host Controller Interface driver

...which I don't understand; did not we say warn on _once_?
... Um. But I think we have a winner: "lguest: mapped switcher at
ffe69000".

Rusty, does the switcher need to be W+X?

And yes, I have lguest enabled, not sure why. 

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292701 — Re: 4.4.-rc5: lguest causes ugly warn on: 5 W+X pages found

Pavel Machek <pavel@ucw.cz> writes:
> Hi!
>
>> > or similar?
>> > 
>> > The above is entirely untested. Maybe it doesn't compile. Or
>> > boot. Or work.
>> 
>> Well, with two extra spaces at each line, it does not apply :-).
>> 
>> I applied it by hand, and the output is:
>> 
>> [    0.000000] MTRR variable ranges enabled:
> ...> [    0.000000] BRK [0x0566c000, 0x0566cfff] PGTABLE
>> 
>> I'll take a look if I can figure out what it means...
>
> Wait, there's more in the log.
>
> [    1.952146] Bluetooth: HCI UART protocol H4 registered
> [    1.954335] Bluetooth: HCI UART protocol BCSP registered
> [    1.956750] usbcore: registered new interface driver btusb
> [    1.958953] ------------[ cut here ]------------
> [    1.961149] WARNING: CPU: 1 PID: 1 at
> ./arch/x86/include/asm/pgtable.h:357
> vmap_page_range_noflush+0x1f0/0x280()
> [    1.963511] Modules linked in:
> [    1.965849] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W
> 4.4.0-rc5+ #137
> [    1.968230] Hardware name: LENOVO 17097HU/17097HU, BIOS 7BETD8WW
> (2.19 ) 03/31/2011
> [    1.970593]  00000001 00000000 f5cffe64 c42baaf8 00000000 f5cffe80
> c404066b 00000165
> [    1.973103]  c40fbe70 00000163 00000000 00000000 f5cffe90 c404070f
> 00000009 00000000
> [    1.975670]  f5cffee0 c40fbe70 c4f88348 00000000 ffe6dfff ffe6e000
> c4f8a018 ffe6dfff
> [    1.978304] Call Trace:
> [    1.980882]  [<c42baaf8>] dump_stack+0x41/0x59
> [    1.983464]  [<c404066b>] warn_slowpath_common+0x6b/0xa0
> [    1.986053]  [<c40fbe70>] ? vmap_page_range_noflush+0x1f0/0x280
> [    1.988625]  [<c404070f>] warn_slowpath_null+0xf/0x20
> [    1.991154]  [<c40fbe70>] vmap_page_range_noflush+0x1f0/0x280
> [    1.993676]  [<c40fbf2b>] map_vm_area+0x2b/0x40
> [    1.996153]  [<c4f2c795>] init+0xf8/0x1a4
> [    1.998591]  [<c4f2c69d>] ? edac_init+0x67/0x67
> [    2.001014]  [<c4000442>] do_one_initcall+0xc2/0x1c0
> [    2.003391]  [<c4f044e3>] ? initcall_blacklist+0x97/0x97
> [    2.005815]  [<c4f044e3>] ? initcall_blacklist+0x97/0x97
> [    2.008161]  [<c4051546>] ?
> __usermodehelper_set_disable_depth+0x36/0x40
> [    2.010518]  [<c407d4a6>] ? up_write+0x16/0x40
> [    2.012817]  [<c4f04ba3>] kernel_init_freeable+0xf0/0x16d
> [    2.015078]  [<c4f04ba3>] ? kernel_init_freeable+0xf0/0x16d
> [    2.017386]  [<c4a4d9c8>] kernel_init+0x8/0xc0
> [    2.019661]  [<c4a54149>] ret_from_kernel_thread+0x21/0x38
> [    2.021932]  [<c4a4d9c0>] ? rest_init+0xa0/0xa0
> [    2.024168] ---[ end trace e117245cd61feaf2 ]---
> [    2.026383] lguest: mapped switcher at ffe69000
> [    2.028958] sdhci: Secure Digital Host Controller Interface driver
>
> ...which I don't understand; did not we say warn on _once_?
> ... Um. But I think we have a winner: "lguest: mapped switcher at
> ffe69000".
>
> Rusty, does the switcher need to be W+X?
>
> And yes, I have lguest enabled, not sure why. 

No.  The layout is "<text page> <per-cpu-stack-pages>..." and I lazily
did that as a single
        map_vm_area(switcher_vma, PAGE_KERNEL_EXEC, lg_switcher_pages);

This boots, does it solve the problem?

Thanks!
Rusty.

From: Rusty Russell <rusty@rustcorp.com.au>
Subject: lguest: map switcher text R/O.

Pavel noted that lguest maps the switcher code executable and
read-write.  This is a bad idea for any kernel text, but particularly
for text mapped at a fixed address.

Create two vmas, one for the text (PAGE_KERNEL_RX) and another for the
stacks (PAGE_KERNEL).  Use VM_NO_GUARD to map them adjacent (as
expected by the rest of the code).

Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/arch/x86/include/asm/lguest.h b/arch/x86/include/asm/lguest.h
index 3bbc07a57a31..73d0c9b92087 100644
--- a/arch/x86/include/asm/lguest.h
+++ b/arch/x86/include/asm/lguest.h
@@ -12,7 +12,9 @@
 #define GUEST_PL 1
 
 /* Page for Switcher text itself, then two pages per cpu */
-#define TOTAL_SWITCHER_PAGES (1 + 2 * nr_cpu_ids)
+#define SWITCHER_TEXT_PAGES (1)
+#define SWITCHER_STACK_PAGES (2 * nr_cpu_ids)
+#define TOTAL_SWITCHER_PAGES (SWITCHER_TEXT_PAGES + SWITCHER_STACK_PAGES)
 
 /* Where we map the Switcher, in both Host and Guest. */
 extern unsigned long switcher_addr;
diff --git a/drivers/lguest/core.c b/drivers/lguest/core.c
index 312ffd3d0017..021915baef35 100644
--- a/drivers/lguest/core.c
+++ b/drivers/lguest/core.c
@@ -22,7 +22,8 @@
 
 unsigned long switcher_addr;
 struct page **lg_switcher_pages;
-static struct vm_struct *switcher_vma;
+static struct vm_struct *switcher_text_vma;
+static struct vm_struct *switcher_stacks_vma;
 
 /* This One Big lock protects all inter-guest data structures. */
 DEFINE_MUTEX(lguest_lock);
@@ -83,54 +84,80 @@ static __init int map_switcher(void)
 	}
 
 	/*
+	 * Copy in the compiled-in Switcher code (from x86/switcher_32.S).
+	 * It goes in the first page, which we map in momentarily.
+	 */
+	memcpy(kmap(lg_switcher_pages[0]), start_switcher_text,
+	       end_switcher_text - start_switcher_text);
+	kunmap(lg_switcher_pages[0]);
+
+	/*
 	 * We place the Switcher underneath the fixmap area, which is the
 	 * highest virtual address we can get.  This is important, since we
 	 * tell the Guest it can't access this memory, so we want its ceiling
 	 * as high as possible.
 	 */
-	switcher_addr = FIXADDR_START - (TOTAL_SWITCHER_PAGES+1)*PAGE_SIZE;
+	switcher_addr = FIXADDR_START - TOTAL_SWITCHER_PAGES*PAGE_SIZE;
 
 	/*
-	 * Now we reserve the "virtual memory area" we want.  We might
-	 * not get it in theory, but in practice it's worked so far.
-	 * The end address needs +1 because __get_vm_area allocates an
-	 * extra guard page, so we need space for that.
+	 * Now we reserve the "virtual memory area"s we want.  We might
+	 * not get them in theory, but in practice it's worked so far.
+	 *
+	 * We want the switcher text to be read-only and executable, and
+	 * the stacks to be read-write and non-executable.
 	 */
-	switcher_vma = __get_vm_area(TOTAL_SWITCHER_PAGES * PAGE_SIZE,
-				     VM_ALLOC, switcher_addr, switcher_addr
-				     + (TOTAL_SWITCHER_PAGES+1) * PAGE_SIZE);
-	if (!switcher_vma) {
+	switcher_text_vma = __get_vm_area(PAGE_SIZE, VM_ALLOC|VM_NO_GUARD,
+					  switcher_addr,
+					  switcher_addr + PAGE_SIZE);
+
+	if (!switcher_text_vma) {
 		err = -ENOMEM;
 		printk("lguest: could not map switcher pages high\n");
 		goto free_pages;
 	}
 
+	switcher_stacks_vma = __get_vm_area(SWITCHER_STACK_PAGES * PAGE_SIZE,
+					    VM_ALLOC|VM_NO_GUARD,
+					    switcher_addr + PAGE_SIZE,
+					    switcher_addr + TOTAL_SWITCHER_PAGES * PAGE_SIZE);
+	if (!switcher_stacks_vma) {
+		err = -ENOMEM;
+		printk("lguest: could not map switcher pages high\n");
+		goto free_text_vma;
+	}
+
 	/*
 	 * This code actually sets up the pages we've allocated to appear at
 	 * switcher_addr.  map_vm_area() takes the vma we allocated above, the
-	 * kind of pages we're mapping (kernel pages), and a pointer to our
-	 * array of struct pages.
+	 * kind of pages we're mapping (kernel text pages and kernel writable
+	 * pages respectively), and a pointer to our array of struct pages.
 	 */
-	err = map_vm_area(switcher_vma, PAGE_KERNEL_EXEC, lg_switcher_pages);
+	err = map_vm_area(switcher_text_vma, PAGE_KERNEL_RX, lg_switcher_pages);
+	if (err) {
+		printk("lguest: text map_vm_area failed: %i\n", err);
+		goto free_vmas;
+	}
+
+	err = map_vm_area(switcher_stacks_vma, PAGE_KERNEL,
+			  lg_switcher_pages + SWITCHER_TEXT_PAGES);
 	if (err) {
-		printk("lguest: map_vm_area failed: %i\n", err);
-		goto free_vma;
+		printk("lguest: stacks map_vm_area failed: %i\n", err);
+		goto free_vmas;
 	}
 
 	/*
 	 * Now the Switcher is mapped at the right address, we can't fail!
-	 * Copy in the compiled-in Switcher code (from x86/switcher_32.S).
 	 */
-	memcpy(switcher_vma->addr, start_switcher_text,
-	       end_switcher_text - start_switcher_text);
-
 	printk(KERN_INFO "lguest: mapped switcher at %p\n",
-	       switcher_vma->addr);
+	       switcher_text_vma->addr);
 	/* And we succeeded... */
 	return 0;
 
-free_vma:
-	vunmap(switcher_vma->addr);
+free_vmas:
+	/* Undoes map_vm_area and __get_vm_area */ 
+	vunmap(switcher_stacks_vma->addr);
+free_text_vma:
+	vunmap(switcher_text_vma->addr);
 free_pages:
 	i = TOTAL_SWITCHER_PAGES;
 free_some_pages:
@@ -148,7 +175,8 @@ static void unmap_switcher(void)
 	unsigned int i;
 
 	/* vunmap() undoes *both* map_vm_area() and __get_vm_area(). */
-	vunmap(switcher_vma->addr);
+	vunmap(switcher_text_vma->addr);
+	vunmap(switcher_stacks_vma->addr);
 	/* Now we just need to free the pages we copied the switcher into */
 	for (i = 0; i < TOTAL_SWITCHER_PAGES; i++)
 		__free_pages(lg_switcher_pages[i], 0);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1292814 — Re: 4.4.-rc5: lguest causes ugly warn on: 5 W+X pages found

Hi!

> > Rusty, does the switcher need to be W+X?
> >
> > And yes, I have lguest enabled, not sure why. 
> 
> No.  The layout is "<text page> <per-cpu-stack-pages>..." and I lazily
> did that as a single
>         map_vm_area(switcher_vma, PAGE_KERNEL_EXEC, lg_switcher_pages);
> 
> This boots, does it solve the problem?

Let me see. Note that I'm not actually using lguest.

git complains about trailing whitespace in the patch.

Otherwise... yes, this helps. "no W+X pages found".

Tested-by: Pavel Machek <pavel@ucw.cz>

Thanks,
								Pavel

> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/arch/x86/include/asm/lguest.h b/arch/x86/include/asm/lguest.h
> index 3bbc07a57a31..73d0c9b92087 100644
> --- a/arch/x86/include/asm/lguest.h
> +++ b/arch/x86/include/asm/lguest.h
> @@ -12,7 +12,9 @@
>  #define GUEST_PL 1
>  
>  /* Page for Switcher text itself, then two pages per cpu */
> -#define TOTAL_SWITCHER_PAGES (1 + 2 * nr_cpu_ids)
> +#define SWITCHER_TEXT_PAGES (1)
> +#define SWITCHER_STACK_PAGES (2 * nr_cpu_ids)
> +#define TOTAL_SWITCHER_PAGES (SWITCHER_TEXT_PAGES + SWITCHER_STACK_PAGES)
>  
>  /* Where we map the Switcher, in both Host and Guest. */
>  extern unsigned long switcher_addr;
> diff --git a/drivers/lguest/core.c b/drivers/lguest/core.c
> index 312ffd3d0017..021915baef35 100644
> --- a/drivers/lguest/core.c
> +++ b/drivers/lguest/core.c
> @@ -22,7 +22,8 @@
>  
>  unsigned long switcher_addr;
>  struct page **lg_switcher_pages;
> -static struct vm_struct *switcher_vma;
> +static struct vm_struct *switcher_text_vma;
> +static struct vm_struct *switcher_stacks_vma;
>  
>  /* This One Big lock protects all inter-guest data structures. */
>  DEFINE_MUTEX(lguest_lock);
> @@ -83,54 +84,80 @@ static __init int map_switcher(void)
>  	}
>  
>  	/*
> +	 * Copy in the compiled-in Switcher code (from x86/switcher_32.S).
> +	 * It goes in the first page, which we map in momentarily.
> +	 */
> +	memcpy(kmap(lg_switcher_pages[0]), start_switcher_text,
> +	       end_switcher_text - start_switcher_text);
> +	kunmap(lg_switcher_pages[0]);
> +
> +	/*
>  	 * We place the Switcher underneath the fixmap area, which is the
>  	 * highest virtual address we can get.  This is important, since we
>  	 * tell the Guest it can't access this memory, so we want its ceiling
>  	 * as high as possible.
>  	 */
> -	switcher_addr = FIXADDR_START - (TOTAL_SWITCHER_PAGES+1)*PAGE_SIZE;
> +	switcher_addr = FIXADDR_START - TOTAL_SWITCHER_PAGES*PAGE_SIZE;
>  
>  	/*
> -	 * Now we reserve the "virtual memory area" we want.  We might
> -	 * not get it in theory, but in practice it's worked so far.
> -	 * The end address needs +1 because __get_vm_area allocates an
> -	 * extra guard page, so we need space for that.
> +	 * Now we reserve the "virtual memory area"s we want.  We might
> +	 * not get them in theory, but in practice it's worked so far.
> +	 *
> +	 * We want the switcher text to be read-only and executable, and
> +	 * the stacks to be read-write and non-executable.
>  	 */
> -	switcher_vma = __get_vm_area(TOTAL_SWITCHER_PAGES * PAGE_SIZE,
> -				     VM_ALLOC, switcher_addr, switcher_addr
> -				     + (TOTAL_SWITCHER_PAGES+1) * PAGE_SIZE);
> -	if (!switcher_vma) {
> +	switcher_text_vma = __get_vm_area(PAGE_SIZE, VM_ALLOC|VM_NO_GUARD,
> +					  switcher_addr,
> +					  switcher_addr + PAGE_SIZE);
> +
> +	if (!switcher_text_vma) {
>  		err = -ENOMEM;
>  		printk("lguest: could not map switcher pages high\n");
>  		goto free_pages;
>  	}
>  
> +	switcher_stacks_vma = __get_vm_area(SWITCHER_STACK_PAGES * PAGE_SIZE,
> +					    VM_ALLOC|VM_NO_GUARD,
> +					    switcher_addr + PAGE_SIZE,
> +					    switcher_addr + TOTAL_SWITCHER_PAGES * PAGE_SIZE);
> +	if (!switcher_stacks_vma) {
> +		err = -ENOMEM;
> +		printk("lguest: could not map switcher pages high\n");
> +		goto free_text_vma;
> +	}
> +
>  	/*
>  	 * This code actually sets up the pages we've allocated to appear at
>  	 * switcher_addr.  map_vm_area() takes the vma we allocated above, the
> -	 * kind of pages we're mapping (kernel pages), and a pointer to our
> -	 * array of struct pages.
> +	 * kind of pages we're mapping (kernel text pages and kernel writable
> +	 * pages respectively), and a pointer to our array of struct pages.
>  	 */
> -	err = map_vm_area(switcher_vma, PAGE_KERNEL_EXEC, lg_switcher_pages);
> +	err = map_vm_area(switcher_text_vma, PAGE_KERNEL_RX, lg_switcher_pages);
> +	if (err) {
> +		printk("lguest: text map_vm_area failed: %i\n", err);
> +		goto free_vmas;
> +	}
> +
> +	err = map_vm_area(switcher_stacks_vma, PAGE_KERNEL,
> +			  lg_switcher_pages + SWITCHER_TEXT_PAGES);
>  	if (err) {
> -		printk("lguest: map_vm_area failed: %i\n", err);
> -		goto free_vma;
> +		printk("lguest: stacks map_vm_area failed: %i\n", err);
> +		goto free_vmas;
>  	}
>  
>  	/*
>  	 * Now the Switcher is mapped at the right address, we can't fail!
> -	 * Copy in the compiled-in Switcher code (from x86/switcher_32.S).
>  	 */
> -	memcpy(switcher_vma->addr, start_switcher_text,
> -	       end_switcher_text - start_switcher_text);
> -
>  	printk(KERN_INFO "lguest: mapped switcher at %p\n",
> -	       switcher_vma->addr);
> +	       switcher_text_vma->addr);
>  	/* And we succeeded... */
>  	return 0;
>  
> -free_vma:
> -	vunmap(switcher_vma->addr);
> +free_vmas:
> +	/* Undoes map_vm_area and __get_vm_area */ 
> +	vunmap(switcher_stacks_vma->addr);
> +free_text_vma:
> +	vunmap(switcher_text_vma->addr);
>  free_pages:
>  	i = TOTAL_SWITCHER_PAGES;
>  free_some_pages:
> @@ -148,7 +175,8 @@ static void unmap_switcher(void)
>  	unsigned int i;
>  
>  	/* vunmap() undoes *both* map_vm_area() and __get_vm_area(). */
> -	vunmap(switcher_vma->addr);
> +	vunmap(switcher_text_vma->addr);
> +	vunmap(switcher_stacks_vma->addr);
>  	/* Now we just need to free the pages we copied the switcher into */
>  	for (i = 0; i < TOTAL_SWITCHER_PAGES; i++)
>  		__free_pages(lg_switcher_pages[i], 0);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web