On Fri, 18 Jun 2021 at 19:30, Maciej Żenczykowski
[off-list ref] wrote:
On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer [off-list ref] wrote:
quoted
On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
[off-list ref] wrote:
quoted
This reverts commit d37300ed182131f1757895a62e556332857417e5.
This breaks Android userspace which expects to be able to
fetch programs with just read permissions.
Sorry about this! I'll defer to the maintainers what to do here.
Reverting leaves us with a gaping hole for access control of pinned
programs.
Not sure what hole you're referring to. Could you provide more details/explanation?
It seems perfectly reasonable to be able to get a program with just read privs.
After all, you're not modifying it, just using it.
Agreed, if that was what the kernel is doing. What you get with
BPF_F_RDONLY is a fully read-write fd, since the rest of the BPF
subsystem doesn't check program fd flags. Hence my fix to only allow
O_RDWR, which matches what the kernel actually does. Otherwise any
user with read-only access can get a R/W fd.
AFAIK there is no way to modify a program after it was loaded, has this changed?
You can't modify the program, but you can detach it, for example. Any
program related bpf command that takes a program fd basically.
if so, the checks should be on the modifications not the fd fetch.
True, unfortunately that code doesn't exist. It's also not
straightforward to write and probably impossible to backport.
I guess one could argue fetching with write only privs doesn't make sense?
Anyway... userspace is broken... so revert is the answer.
In Android the process loading/pinning bpf maps/programs is a different
process (the 'bpfloader') to the users (which are far less privileged)
If the revert happens you need to make sure that all of your pinned
state is only readable by the bpfloader user. And everybody else,
realistically.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
The patch this reverts breaks (Android) userspace, and I've even
pointed out the specific code in question that it breaks.
What happened to the policy of not breaking userspace?
Why are 'Changes Requested' (see
https://patchwork.kernel.org/project/netdevbpf/patch/20210618105526.265003-1-zenczykowski@gmail.com/
)
by whom? What changes? What do you expect me to do?
Why should I even care? Why should it even be me? I'm not the one
that broke things.
On Mon, Jun 21, 2021 at 2:02 AM Lorenz Bauer [off-list ref] wrote:
On Fri, 18 Jun 2021 at 19:30, Maciej Żenczykowski
[off-list ref] wrote:
quoted
On Fri, Jun 18, 2021 at 4:55 AM Lorenz Bauer [off-list ref] wrote:
quoted
On Fri, 18 Jun 2021 at 11:55, Maciej Żenczykowski
[off-list ref] wrote:
quoted
This reverts commit d37300ed182131f1757895a62e556332857417e5.
This breaks Android userspace which expects to be able to
fetch programs with just read permissions.
Sorry about this! I'll defer to the maintainers what to do here.
Reverting leaves us with a gaping hole for access control of pinned
programs.
Not sure what hole you're referring to. Could you provide more details/explanation?
It seems perfectly reasonable to be able to get a program with just read privs.
After all, you're not modifying it, just using it.
Agreed, if that was what the kernel is doing. What you get with
BPF_F_RDONLY is a fully read-write fd, since the rest of the BPF
subsystem doesn't check program fd flags. Hence my fix to only allow
O_RDWR, which matches what the kernel actually does. Otherwise any
user with read-only access can get a R/W fd.
quoted
AFAIK there is no way to modify a program after it was loaded, has this changed?
You can't modify the program, but you can detach it, for example. Any
program related bpf command that takes a program fd basically.
I fail to see how this is a problem, since it's not modifying the program,
why should it need a rdwr file descriptor to do so?
AFAIK in many cases, you don't even need the bpf file descriptor at
all (for example
you can detach a tc bpf program via removing the tc filter or tc qdisc
or the network interface).
Do you perhaps mean to say you can unpin it?
But, if so, then that's a problem in the unpin code...
Or it's even entirely unrelated, since deleting files does not need
read or write access to the file, just the folder the file is in.
[maze@zeus ~]$ touch foo; chmod a-rwx foo; sudo chown root:root foo; ls -al foo
----------. 1 root root 0 Jun 21 14:26 foo
[maze@zeus ~]$ rm -f foo; ls -al foo
ls: cannot access 'foo': No such file or directory
quoted
if so, the checks should be on the modifications not the fd fetch.
True, unfortunately that code doesn't exist. It's also not
straightforward to write and probably impossible to backport.
Now you're suggesting you expect this broken patch (that I'm trying to revert)
to make it into older LTS releases and break things out in the field???
quoted
I guess one could argue fetching with write only privs doesn't make sense?
Anyway... userspace is broken... so revert is the answer.
In Android the process loading/pinning bpf maps/programs is a different
process (the 'bpfloader') to the users (which are far less privileged)
If the revert happens you need to make sure that all of your pinned
state is only readable by the bpfloader user. And everybody else,
realistically.
On Android selinux prevents anyone from doing untoward things, since
they can't get the fds in the first place.
I *want* less privileged users (that can't load bpf programs, but have
the selinux privs to get pinned program fds) to be able to
attach/detach them (via xdp, tc or to cgroups). That's how stuff works
right *now*.
Could I perhaps redesign the system to work around this?
I don't know. Perhaps. Perhaps not.
I haven't given it that much thought - I'm still trying to fix
(workaround) an hrtimer ncm performance regression that was introduced
in 5.10.~24 LTS (there at least we can argue the old code was buggy,
and the hrtimer implementation on the hardware in question outright
terrible and super slow).
I'm guessing changes would be needed to the progRetrieve() function,
and the tests, and the bpfloader, and the permissions embedded in the
programs themselves, and possibly the iptables binary (and the
netutils wrapper) since it uses xt_bpf, and possibly to binaries
privileges and/or selinux capabilities/policies. Or maybe not. I
simply don't know.
Well tested patches to make things work are welcome at the aosp project.
A word of warning: I haven't checked / thought things through, but it
may take 5+ years to roll them out.
At this point it's far too late to make such changes in Android 12/S.
Here's another example of fetching programs with BPF_F_RDONLY in
iptables (yes, it was added by me, due to our use in Android):
https://android.googlesource.com/platform/external/iptables.git/+/refs/heads/master/extensions/libxt_bpf.c#64
The fact that this snuck into the 5.12 final release is not relevant
(ie. this is a regression in 5.12 vs 5.11, and it is still broken in
5.13-rcX).
Please revert immediately. I've got better things to do. I shouldn't
have to be thinking about this or arguing about this.
It already took me significantly more than a day simply to track this
down (arguably due to miscommunications with Greg, who'd earlier
actually found this in 5.12, but misunderstood the problem, but
still...).
Thanks,
Maciej
On Mon, 21 Jun 2021 at 22:37, Maciej Żenczykowski
[off-list ref] wrote:
Please revert immediately. I've got better things to do. I shouldn't
have to be thinking about this or arguing about this.
It already took me significantly more than a day simply to track this
down (arguably due to miscommunications with Greg, who'd earlier
actually found this in 5.12, but misunderstood the problem, but
still...).
You're barking up the wrong tree. I don't object to reverting the
patch, you asked me for context and I gave it to you.
Best
Lorenz
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
On 6/22/21 10:59 AM, Lorenz Bauer wrote:
On Mon, 21 Jun 2021 at 22:37, Maciej Żenczykowski
[off-list ref] wrote:
quoted
Please revert immediately. I've got better things to do. I shouldn't
have to be thinking about this or arguing about this.
It already took me significantly more than a day simply to track this
down (arguably due to miscommunications with Greg, who'd earlier
actually found this in 5.12, but misunderstood the problem, but
still...).
You're barking up the wrong tree. I don't object to reverting the
patch, you asked me for context and I gave it to you.
+1, this kind of barking was unnecessary and inappropriate.
I revamped the commit message a bit to have some more context for future
reference when we need to get back on this.
Anyway, applied.
quoted
You're barking up the wrong tree. I don't object to reverting the
patch, you asked me for context and I gave it to you.
+1, this kind of barking was unnecessary and inappropriate.
I revamped the commit message a bit to have some more context for future
reference when we need to get back on this.
Anyway, applied.
Thank you for applying the change.
Sorry for the tone, but work has been particularly hectic these past
few quarters, with a constant onslaught of neverending deadlines...
The current kernel patch management system provides virtually no
feedback about (rejected) patches.
There is no email when a patch is rejected (and often even when
applied) and there is no notification of who did it or why.
It's very much like dealing with a faceless robot.
I'm stuck periodically refreshing a browser tab with
https://patchwork.kernel.org/project/netdevbpf/list/ and waiting on
patch state to change from new to accepted or 'changes requested'. In
this particular case, there was no email feedback
from any maintainer, except for the patch going into 'changes
requested' state on patchworks, and the email thread (which only had
me and Lorenz participating) also didn't appear to have any reason in
it either....
As such, it was extremely unclear what was being asked of me.
It felt like I was simply being ignored.
- Maciej