Re: [PATCH v2] exec: Correct the permission check for unsafe exec

["Eric W. Biederman" <ebiederm@xmission.com>]

Max Kellermann <max.kellermann@ionos.com> writes:

> On Wed, Jun 11, 2025 at 2:19 AM Paul Moore <paul@paul-moore.com> wrote:
>> Aside from a tested-by verification from Max, it looks like everyone
>> is satisfied with the v2 patch, yes?
>
> Sorry for the delay. I tested Eric's v2 patch and it solves my
> problem. His patch is nearly identical to mine, it's only a bit more
> intrusive by removing the weird __is_setXid functions that never made
> sense. I welcome that; I wasn't confident enough to do that and tried
> to make the least intrusive patch.
>
> Eric, I'm glad you changed your mind and no longer consider my work
> "pure nonsense" and "pointless".

As you pointed out in that case my analysis of your code was incorrect.

Further I wrote this patch when I finally realized what is going on and
that the case you are dealing with is an actual bug in the current
code and not some kind of enhancement or extension.

> But one problem remains: in the same email, you demanded evidence that
> userspace doesn't depend on the current behavior. However, in your
> patch description, you hand-waved that away by "I don't expect anyone
> to care". What happened to that?


The analysis of __is_setuid and __is_setgid that allowed me to remove
them helped quite a lot.

The analysis makes it clear that the code change is semantically safe
so we don't loose anything by not mucking with permissions.

The analysis shows the code is good comprehension and maintenance and
not just for your case.  It also makes it clear why not supporting your
case is a bug, and frankly a regression in the current code. (A 20 year
old regression so that doesn't carry much weight but still a
regression).

A related analysis in another parallel thread mostly concluded that for
brpm->unsafe in general it is a better user experience to terminate the
exec with a permission error instead of continuing the exec.  Exception
ptrace.

Part of my resistance was the initial reading that your change was
trying to escape the unsafe downgrading of permissions, instead of
showing that it was safe to keep the permissions.

With the reminder that no one should even be exercising the permission
downgrading case, it became clear that changing the little bit that is
safe should not affect may users at all.

Failing the exec rather than downgrading permissions will also make
a good test to see if anyone cares about this functionality.  I do still
believe we should tread carefully.

Hopefully that makes things clear.

Eric