| Message ID | 20221115175652.3836811-5-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security: Ensure LSMs return expected values | expand |
On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > 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. 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.
On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote: > On Tue, Nov 15, 2022 at 12:58 PM Roberto Sassu > <roberto.sassu@huaweicloud.com> wrote: > > 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
On Wed, Nov 16, 2022 at 9:37 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Tue, 2022-11-15 at 21:35 -0500, Paul Moore wrote: > > 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, I think that might be the best approach. Also, please resubmit patches 1/4 and 2/4 with those small changes; those are nice improvements that just need a couple of small tweaks to be acceptable :)
diff --git a/security/security.c b/security/security.c index 4041d24e3283..cd417a8a0e65 100644 --- a/security/security.c +++ b/security/security.c @@ -716,6 +716,35 @@ static int lsm_superblock_alloc(struct super_block *sb) #include <linux/lsm_hook_defs.h> #undef LSM_HOOK +/* + * The return value flags of the LSM hook are defined in linux/lsm_hook_defs.h + * and can be accessed with: + * + * LSM_RET_FLAGS(<hook_name>) + * + * The macros below define static constants for the return value flags of each + * LSM hook. + */ +#define LSM_RET_FLAGS(NAME) (NAME##_ret_flags) +#define DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME) \ + static const u32 __maybe_unused LSM_RET_FLAGS(NAME) = (RET_FLAGS); +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...) \ + DECLARE_LSM_RET_FLAGS(RET_FLAGS, NAME) + +#include <linux/lsm_hook_defs.h> +#undef LSM_HOOK + +static bool is_ret_value_allowed(int ret, u32 ret_flags) +{ + if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) || + (ret == 0 && !(ret_flags & LSM_RET_ZERO)) || + (ret == 1 && !(ret_flags & LSM_RET_ONE)) || + (ret > 1 && !(ret_flags & LSM_RET_GT_ONE))) + return false; + + return true; +} + /* * Hook list operation macros. * @@ -741,6 +770,11 @@ static int lsm_superblock_alloc(struct super_block *sb) \ hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \ RC = P->hook.FUNC(__VA_ARGS__); \ + if (!is_ret_value_allowed(RC, LSM_RET_FLAGS(FUNC))) { \ + WARN_ONCE(1, "Illegal ret %d for " #FUNC " from %s, forcing %d\n", \ + RC, P->lsm, LSM_RET_DEFAULT(FUNC)); \ + RC = LSM_RET_DEFAULT(FUNC); \ + } \ if (RC != 0) \ break; \ } \