Re: [syzbot] WARNING in __change_page_attr_set_clr

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Dave Hansen]

On 9/25/22 04:18, syzbot wrote:
> ------------[ cut here ]------------
> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: 0xffffffffa0401000 - 0xffffffffa0401fff PFN 7d8d5
> WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 verify_rwx arch/x86/mm/pat/set_memory.c:600 [inline]
> WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr arch/x86/mm/pat/set_memory.c:1569 [inline]
> WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr_set_clr+0x1f40/0x2020 arch/x86/mm/pat/set_memory.c:1691
> Modules linked in:

Yay, one of these that isn't due to wonky 32-bit kernels!

This one looks to be naughty intentionally:

> void *bpf_jit_alloc_exec_page(void)
> {
...
>         /* Keep image as writeable. The alternative is to keep flipping ro/rw
>          * every time new program is attached or detached.
>          */
>         set_memory_x((long)image, 1);
>         return image;
> }

For STRICT_KERNEL_RWX kernels, I think we would really rather that this
code *did* flip ro/rw every time a new BPF program is attached or detached.

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Alexei Starovoitov]

On Sun, Sep 25, 2022 at 9:44 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 9/25/22 04:18, syzbot wrote:
> > ------------[ cut here ]------------
> > CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: 0xffffffffa0401000 - 0xffffffffa0401fff PFN 7d8d5
> > WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 verify_rwx arch/x86/mm/pat/set_memory.c:600 [inline]
> > WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr arch/x86/mm/pat/set_memory.c:1569 [inline]
> > WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr_set_clr+0x1f40/0x2020 arch/x86/mm/pat/set_memory.c:1691
> > Modules linked in:
>
> Yay, one of these that isn't due to wonky 32-bit kernels!
>
> This one looks to be naughty intentionally:
>
> > void *bpf_jit_alloc_exec_page(void)
> > {
> ...
> >         /* Keep image as writeable. The alternative is to keep flipping ro/rw
> >          * every time new program is attached or detached.
> >          */
> >         set_memory_x((long)image, 1);
> >         return image;
> > }
>
> For STRICT_KERNEL_RWX kernels, I think we would really rather that this
> code *did* flip ro/rw every time a new BPF program is attached or detached.

Steven Rostedt noticed that comment around the middle of August
and told you and Peter about it.
Then Peter added a WARN_ONCE in commit
https://lore.kernel.org/all/YwySW3ROc21hN7g9@hirez.programming.kicks-ass.net/
to explicitly trigger that known issue.
Sure enough the fedora fails to boot on linux-next since then,
because systemd is loading bpf programs that use bpf trampoline.
The boot issue was was reported 3 days ago:
https://lore.kernel.org/bpf/c84cc27c1a5031a003039748c3c099732a718aec.camel@kernel.org/T/#u
Now we're trying to urgently address it with:
https://lore.kernel.org/bpf/20220923211837.3044723-1-song@kernel.org/

So instead of pinging us with your w^x concern you've decided
to fail hard in -next to force the issue and
now acting like this is something surprising to you?!

This is Code of Conduct "worthy" behavior demonstrated
by a newly elected member of the Technical Advisory Board.
Please consider resigning.
A TAB member should be better than this.

As far as this w^x issue...
we've communicated back in May 2022 (sorry I cannot find the link
to that discussion) that bpf_prog_pack is targeting to be used
by everything bpf. Currently by bpf progs only.
bpf trampoline and bpf dispatcher were next on the list.
But then folks expressed the desire to generalize bpf_prog_pack for
everything: bpf, modules and all other trampolines.
We've posted multiple revisions and kept pinging for feedback.
The last one on Aug 24:
https://lkml.org/lkml/2022/8/24/857
If/when the generic vmalloc_exec lands we can finally close
the issue for modules, bpf progs, and trampolines of all kinds.
Make them fast by using large pages and w^x compliant.

And, sorry, "flip ro/rw every time" is not a good idea from
security pov.
There is a much better solution that stalled on the code review.
In the meantime we'll land a quick fix to re-enable boot in -next
in the coming days.

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Theodore Ts'o]

On Sun, Sep 25, 2022 at 02:55:46PM -0700, Alexei Starovoitov wrote:
> 
> So instead of pinging us with your w^x concern you've decided
> to fail hard in -next to force the issue and
> now acting like this is something surprising to you?!
> 
> This is Code of Conduct "worthy" behavior demonstrated
> by a newly elected member of the Technical Advisory Board.

I must be missing something.  Why/what do you think this is
specifically a Code of Conduct violation?

				- Ted

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Randy Dunlap]

On 9/25/22 14:55, Alexei Starovoitov wrote:
> On Sun, Sep 25, 2022 at 9:44 AM Dave Hansen <dave.hansen@intel.com> wrote:
>>
>> On 9/25/22 04:18, syzbot wrote:
>>> ------------[ cut here ]------------
>>> CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: 0xffffffffa0401000 - 0xffffffffa0401fff PFN 7d8d5
>>> WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 verify_rwx arch/x86/mm/pat/set_memory.c:600 [inline]
>>> WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr arch/x86/mm/pat/set_memory.c:1569 [inline]
>>> WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr_set_clr+0x1f40/0x2020 arch/x86/mm/pat/set_memory.c:1691
>>> Modules linked in:
>>
>> Yay, one of these that isn't due to wonky 32-bit kernels!
>>
>> This one looks to be naughty intentionally:
>>
>>> void *bpf_jit_alloc_exec_page(void)
>>> {
>> ...
>>>         /* Keep image as writeable. The alternative is to keep flipping ro/rw
>>>          * every time new program is attached or detached.
>>>          */
>>>         set_memory_x((long)image, 1);
>>>         return image;
>>> }
>>
>> For STRICT_KERNEL_RWX kernels, I think we would really rather that this
>> code *did* flip ro/rw every time a new BPF program is attached or detached.
> 
> Steven Rostedt noticed that comment around the middle of August
> and told you and Peter about it.
> Then Peter added a WARN_ONCE in commit
> https://lore.kernel.org/all/YwySW3ROc21hN7g9@hirez.programming.kicks-ass.net/
> to explicitly trigger that known issue.
> Sure enough the fedora fails to boot on linux-next since then,
> because systemd is loading bpf programs that use bpf trampoline.
> The boot issue was was reported 3 days ago:
> https://lore.kernel.org/bpf/c84cc27c1a5031a003039748c3c099732a718aec.camel@kernel.org/T/#u
> Now we're trying to urgently address it with:
> https://lore.kernel.org/bpf/20220923211837.3044723-1-song@kernel.org/
> 
> So instead of pinging us with your w^x concern you've decided
> to fail hard in -next to force the issue and
> now acting like this is something surprising to you?!
> 
> This is Code of Conduct "worthy" behavior demonstrated
> by a newly elected member of the Technical Advisory Board.
> Please consider resigning.
> A TAB member should be better than this.

If it is (and I don't see it), just file a complaint.
Don't try to be the enforcer.

-- 
~Randy

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Dave Hansen]

On 9/25/22 14:55, Alexei Starovoitov wrote:
> And, sorry, "flip ro/rw every time" is not a good idea from
> security pov.
> There is a much better solution that stalled on the code review.
> In the meantime we'll land a quick fix to re-enable boot in -next
> in the coming days.

Peter, I remember an earlier version of your patch having some various
enforcement modes.  Since the strict enforcement has actually broken a
few things, should we resurrect the nicer soft detection mode?  Or,
maybe make the soft one the only mode for now?

Alexei, the "quick fix" looks sane to me at first glance.  Is there
something in there that's not viable long-term?

Also, the intention here was not to force any issues.  I thought the
earlier discussion resulted in a bpf fix and applying Peter's patch was
intended to catch _future_ issues.  I should have double-checked before
applying it.  My apologies.

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Peter Zijlstra]

On Sun, Sep 25, 2022 at 05:10:19PM -0700, Dave Hansen wrote:

> Peter, I remember an earlier version of your patch having some various
> enforcement modes.  Since the strict enforcement has actually broken a
> few things, should we resurrect the nicer soft detection mode?  Or,
> maybe make the soft one the only mode for now?

Well, I think we'll have to disable the whole thing for i386, but I'm
sincerely hoping this is the only one we'll hit on x86_64 -- I did spend
some effort fixing W^X issues a while back.

Re: [syzbot] WARNING in __change_page_attr_set_clr

[Peter Zijlstra]

On Sun, Sep 25, 2022 at 02:55:46PM -0700, Alexei Starovoitov wrote:

> Steven Rostedt noticed that comment around the middle of August
> and told you and Peter about it.

He did indeed; and I was thinking he'd told you about it too so you
could fix, what is a very juicy security issue, ASAP.

> Then Peter added a WARN_ONCE in commit
> https://lore.kernel.org/all/YwySW3ROc21hN7g9@hirez.programming.kicks-ass.net/
> to explicitly trigger that known issue.

Well, I had sincerely hoped you'd fixed it by now. You just don't let
things like that slide. Note how I didn't post that mostly trivial patch
in mid August. Giving you ample time to fix up.

> Now we're trying to urgently address it with:
> https://lore.kernel.org/bpf/20220923211837.3044723-1-song@kernel.org/

Glad to see it being fixed. Thanks!

Re: [Tech-board] [syzbot] WARNING in __change_page_attr_set_clr

[Steven Rostedt]

On Sun, 25 Sep 2022 14:55:46 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Sun, Sep 25, 2022 at 9:44 AM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 9/25/22 04:18, syzbot wrote:  
> > > ------------[ cut here ]------------
> > > CPA refuse W^X violation: 8000000000000163 -> 0000000000000163 range: 0xffffffffa0401000 - 0xffffffffa0401fff PFN 7d8d5
> > > WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 verify_rwx arch/x86/mm/pat/set_memory.c:600 [inline]
> > > WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr arch/x86/mm/pat/set_memory.c:1569 [inline]
> > > WARNING: CPU: 0 PID: 3607 at arch/x86/mm/pat/set_memory.c:600 __change_page_attr_set_clr+0x1f40/0x2020 arch/x86/mm/pat/set_memory.c:1691
> > > Modules linked in:  
> >
> > Yay, one of these that isn't due to wonky 32-bit kernels!
> >
> > This one looks to be naughty intentionally:
> >  
> > > void *bpf_jit_alloc_exec_page(void)
> > > {  
> > ...  
> > >         /* Keep image as writeable. The alternative is to keep flipping ro/rw
> > >          * every time new program is attached or detached.
> > >          */
> > >         set_memory_x((long)image, 1);
> > >         return image;
> > > }  
> >
> > For STRICT_KERNEL_RWX kernels, I think we would really rather that this
> > code *did* flip ro/rw every time a new BPF program is attached or detached.  
> 
> Steven Rostedt noticed that comment around the middle of August
> and told you and Peter about it.
> Then Peter added a WARN_ONCE in commit
> https://lore.kernel.org/all/YwySW3ROc21hN7g9@hirez.programming.kicks-ass.net/
> to explicitly trigger that known issue.
> Sure enough the fedora fails to boot on linux-next since then,
> because systemd is loading bpf programs that use bpf trampoline.
> The boot issue was was reported 3 days ago:
> https://lore.kernel.org/bpf/c84cc27c1a5031a003039748c3c099732a718aec.camel@kernel.org/T/#u
> Now we're trying to urgently address it with:
> https://lore.kernel.org/bpf/20220923211837.3044723-1-song@kernel.org/
> 
> So instead of pinging us with your w^x concern you've decided
> to fail hard in -next to force the issue and
> now acting like this is something surprising to you?!
> 
> This is Code of Conduct "worthy" behavior demonstrated

Here's the link to the Code of Conduct:

https://www.kernel.org/doc/html/latest/process/code-of-conduct.html

Which states:

  Examples of behavior that contributes to creating a positive environment include:

    - Using welcoming and inclusive language
    - Being respectful of differing viewpoints and experiences
    - Gracefully accepting constructive criticism
    - Focusing on what is best for the community
    - Showing empathy towards other community members

  Examples of unacceptable behavior by participants include:

    - The use of sexualized language or imagery and unwelcome sexual attention or advances
    - Trolling, insulting/derogatory comments, and personal or political attacks
    - Public or private harassment
    - Publishing others’ private information, such as a physical or electronic address, without explicit permission
    - Other conduct which could reasonably be considered inappropriate in a professional setting

I do not see how Dave's response is a violation of any of the above.


> by a newly elected member of the Technical Advisory Board.
> Please consider resigning.

Asking someone to resign is a personal attack, and that can be construed as
a violation of the Code of Conduct.

> A TAB member should be better than this.
> 

Let's please keep this on a technical level, as there appears to be a fix
we all can agree on, and let's move forward on that.

Thanks,

-- Steve