On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote:

On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu
[off-list ref] wrote:

quoted

From: Roberto Sassu <roberto.sassu@huawei.com>

LSMs should not be able to return arbitrary return values, as the callers
of the LSM infrastructure might not be ready to handle unexpected values
(e.g. positive values that are first converted to a pointer with ERR_PTR,
and then evaluated with IS_ERR()).

Modify call_int_hook() to call is_ret_value_allowed(), so that the return
value from each LSM for a given hook is checked. If for the interval the
return value falls into the corresponding flag is not set, change the
return value to the default value, just for the current LSM.

A misbehaving LSM would not have impact on the decision of other LSMs, as
the loop terminates whenever the return value is not zero.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/security.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Casey touched on some of this in his reply to patch 0/4, but basically
I see this as a BPF LSM specific problem and not a generalized LSM
issue that should be addressed at the LSM layer.  Especially if the
solution involves incurring additional processing for every LSM hook
instantiation, regardless if a BPF LSM is present.  Reading your
overall patchset description I believe that you understand this too.

Yes, I had this concern too. Thanks Paul and Casey for taking the time
to reply.

I liked the fact that the fix is extremely simple, but nevertheless it
should not impact the performance, if there are alternative ways. I
thought maybe we look at non-zero values, since the check is already
there. But it could be that there is an impact for it too (maybe for
audit_rule_match?).

If you want to somehow instrument the LSM hook definitions (what I
believe to be the motivation behind patch 3/4) to indicate valid
return values for use by the BPF verifier, I think we could entertain
that, or at least discuss it further, but I'm not inclined to support
any runtime overhead at the LSM layer for a specific LSM.

Ok, yes. Patches 1-3 would help to keep in sync the LSM infrastructure
and eBPF, but it is not strictly needed. I could propose an eBPF-only
alternative to declare sets of functions per interval.

More or less, I developed an eBPF-based alternative also for patch 4.
It is just a proof of concept. Will propose it, to validate the idea.

Thanks

Roberto