| Message ID | 160141647786.7997.5490924406329369782.stgit@sifl (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC] lsm,selinux: pass the family information along with xfrm flow | expand |
On 9/29/2020 2:54 PM, Paul Moore wrote: > As pointed out by Herbert in a recent related patch, the LSM hooks > should pass the address family in addition to the xfrm flow as the > family information is needed to safely access the flow. > > While this is not technically a problem for the current LSM/SELinux > code as it only accesses fields common to all address families, we > should still pass the address family so that the LSM hook isn't > inherently flawed. An alternate solution could be to simply pass > the LSM secid instead of flow, but this introduces the problem of > the LSM hook callers sending the wrong secid which would be much > worse. > > Reported-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Paul Moore <paul@paul-moore.com> For what it may be worth Acked-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/lsm_hooks.h | 1 + > include/linux/security.h | 7 +++++-- > net/xfrm/xfrm_state.c | 4 ++-- > security/security.c | 5 +++-- > security/selinux/include/xfrm.h | 3 ++- > security/selinux/xfrm.c | 3 ++- > 7 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 2a8c74d99015..e3c3b5d20469 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -349,7 +349,7 @@ LSM_HOOK(int, 0, xfrm_state_delete_security, struct xfrm_state *x) > LSM_HOOK(int, 0, xfrm_policy_lookup, struct xfrm_sec_ctx *ctx, u32 fl_secid, > u8 dir) > LSM_HOOK(int, 1, xfrm_state_pol_flow_match, struct xfrm_state *x, > - struct xfrm_policy *xp, const struct flowi *fl) > + struct xfrm_policy *xp, const struct flowi *fl, unsigned short family) > LSM_HOOK(int, 0, xfrm_decode_session, struct sk_buff *skb, u32 *secid, > int ckall) > #endif /* CONFIG_SECURITY_NETWORK_XFRM */ > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 9e2e3e63719d..ea088aacfdad 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -1093,6 +1093,7 @@ > * @x contains the state to match. > * @xp contains the policy to check for a match. > * @fl contains the flow to check for a match. > + * @family the flow's address family. > * Return 1 if there is a match. > * @xfrm_decode_session: > * @skb points to skb to decode. > diff --git a/include/linux/security.h b/include/linux/security.h > index 0a0a03b36a3b..701b41eb090c 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -1625,7 +1625,8 @@ void security_xfrm_state_free(struct xfrm_state *x); > int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir); > int security_xfrm_state_pol_flow_match(struct xfrm_state *x, > struct xfrm_policy *xp, > - const struct flowi *fl); > + const struct flowi *fl, > + unsigned short family); > int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid); > void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl); > > @@ -1679,7 +1680,9 @@ static inline int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_s > } > > static inline int security_xfrm_state_pol_flow_match(struct xfrm_state *x, > - struct xfrm_policy *xp, const struct flowi *fl) > + struct xfrm_policy *xp, > + const struct flowi *fl, > + unsigned short family) > { > return 1; > } > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c > index 69520ad3d83b..f90d2f1da44a 100644 > --- a/net/xfrm/xfrm_state.c > +++ b/net/xfrm/xfrm_state.c > @@ -1020,7 +1020,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x, > if (x->km.state == XFRM_STATE_VALID) { > if ((x->sel.family && > !xfrm_selector_match(&x->sel, fl, x->sel.family)) || > - !security_xfrm_state_pol_flow_match(x, pol, fl)) > + !security_xfrm_state_pol_flow_match(x, pol, fl, family)) > return; > > if (!*best || > @@ -1033,7 +1033,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x, > } else if (x->km.state == XFRM_STATE_ERROR || > x->km.state == XFRM_STATE_EXPIRED) { > if (xfrm_selector_match(&x->sel, fl, x->sel.family) && > - security_xfrm_state_pol_flow_match(x, pol, fl)) > + security_xfrm_state_pol_flow_match(x, pol, fl, family)) > *error = -ESRCH; > } > } > diff --git a/security/security.c b/security/security.c > index 70a7ad357bc6..62dd0af7c6bc 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2391,7 +2391,8 @@ int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir) > > int security_xfrm_state_pol_flow_match(struct xfrm_state *x, > struct xfrm_policy *xp, > - const struct flowi *fl) > + const struct flowi *fl, > + unsigned short family) > { > struct security_hook_list *hp; > int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match); > @@ -2407,7 +2408,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, > */ > hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match, > list) { > - rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl); > + rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl, family); > break; > } > return rc; > diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h > index a0b465316292..36907dd06647 100644 > --- a/security/selinux/include/xfrm.h > +++ b/security/selinux/include/xfrm.h > @@ -26,7 +26,8 @@ int selinux_xfrm_state_delete(struct xfrm_state *x); > int selinux_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir); > int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, > struct xfrm_policy *xp, > - const struct flowi *fl); > + const struct flowi *fl, > + unsigned short family); > > #ifdef CONFIG_SECURITY_NETWORK_XFRM > extern atomic_t selinux_xfrm_refcount; > diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c > index 7314196185d1..5beb30237d3a 100644 > --- a/security/selinux/xfrm.c > +++ b/security/selinux/xfrm.c > @@ -175,7 +175,8 @@ int selinux_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir) > */ > int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, > struct xfrm_policy *xp, > - const struct flowi *fl) > + const struct flowi *fl, > + unsigned short family) > { > u32 state_sid; > >
On Tue, 29 Sep 2020, Paul Moore wrote: > As pointed out by Herbert in a recent related patch, the LSM hooks > should pass the address family in addition to the xfrm flow as the > family information is needed to safely access the flow. > > While this is not technically a problem for the current LSM/SELinux > code as it only accesses fields common to all address families, we > should still pass the address family so that the LSM hook isn't > inherently flawed. An alternate solution could be to simply pass > the LSM secid instead of flow, but this introduces the problem of > the LSM hook callers sending the wrong secid which would be much > worse. > > Reported-by: Herbert Xu <herbert@gondor.apana.org.au> > Signed-off-by: Paul Moore <paul@paul-moore.com> I'm not keen on adding a parameter which nobody is using. Perhaps a note in the header instead?
On Wed, Sep 30, 2020 at 09:09:20AM +1000, James Morris wrote: > > I'm not keen on adding a parameter which nobody is using. Perhaps a note > in the header instead? Please at least change to the struct flowi to flowi_common if we're not adding a family field. Thanks,
On Tue, Sep 29, 2020 at 7:09 PM James Morris <jmorris@namei.org> wrote: > I'm not keen on adding a parameter which nobody is using. Perhaps a note > in the header instead? On Wed, Sep 30, 2020 at 6:14 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > Please at least change to the struct flowi to flowi_common if we're > not adding a family field. It did feel a bit weird adding a (currently) unused parameter, so I can understand the concern, I just worry that a comment in the code will be easily overlooked. I also thought about passing a pointer to the nested flowi_common struct, but it doesn't appear that this is done anywhere else in the stack so it felt wrong to do it here.
On Wed, Sep 30, 2020 at 9:44 AM Paul Moore <paul@paul-moore.com> wrote: > On Tue, Sep 29, 2020 at 7:09 PM James Morris <jmorris@namei.org> wrote: > > I'm not keen on adding a parameter which nobody is using. Perhaps a note > > in the header instead? > > On Wed, Sep 30, 2020 at 6:14 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > Please at least change to the struct flowi to flowi_common if we're > > not adding a family field. > > It did feel a bit weird adding a (currently) unused parameter, so I > can understand the concern, I just worry that a comment in the code > will be easily overlooked. I also thought about passing a pointer to > the nested flowi_common struct, but it doesn't appear that this is > done anywhere else in the stack so it felt wrong to do it here. With the merge window behind us, where do stand on this? I see the ACK from Casey and some grumbling about adding an unused parameter (which is a valid argument, I just feel the alternative is worse), but I haven't seen any serious NACKs. Any objections or other strong feelings to me merging this via the selinux/next branch?
On Tue, 27 Oct 2020, Paul Moore wrote: > On Wed, Sep 30, 2020 at 9:44 AM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Sep 29, 2020 at 7:09 PM James Morris <jmorris@namei.org> wrote: > > > I'm not keen on adding a parameter which nobody is using. Perhaps a note > > > in the header instead? > > > > On Wed, Sep 30, 2020 at 6:14 AM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > Please at least change to the struct flowi to flowi_common if we're > > > not adding a family field. > > > > It did feel a bit weird adding a (currently) unused parameter, so I > > can understand the concern, I just worry that a comment in the code > > will be easily overlooked. I also thought about passing a pointer to > > the nested flowi_common struct, but it doesn't appear that this is > > done anywhere else in the stack so it felt wrong to do it here. > > With the merge window behind us, where do stand on this? I see the > ACK from Casey and some grumbling about adding an unused parameter > (which is a valid argument, I just feel the alternative is worse), but > I haven't seen any serious NACKs. > > Any objections or other strong feelings to me merging this via the > selinux/next branch? Yes, we should not add unused parameters to functions.
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 2a8c74d99015..e3c3b5d20469 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -349,7 +349,7 @@ LSM_HOOK(int, 0, xfrm_state_delete_security, struct xfrm_state *x) LSM_HOOK(int, 0, xfrm_policy_lookup, struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir) LSM_HOOK(int, 1, xfrm_state_pol_flow_match, struct xfrm_state *x, - struct xfrm_policy *xp, const struct flowi *fl) + struct xfrm_policy *xp, const struct flowi *fl, unsigned short family) LSM_HOOK(int, 0, xfrm_decode_session, struct sk_buff *skb, u32 *secid, int ckall) #endif /* CONFIG_SECURITY_NETWORK_XFRM */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 9e2e3e63719d..ea088aacfdad 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1093,6 +1093,7 @@ * @x contains the state to match. * @xp contains the policy to check for a match. * @fl contains the flow to check for a match. + * @family the flow's address family. * Return 1 if there is a match. * @xfrm_decode_session: * @skb points to skb to decode. diff --git a/include/linux/security.h b/include/linux/security.h index 0a0a03b36a3b..701b41eb090c 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1625,7 +1625,8 @@ void security_xfrm_state_free(struct xfrm_state *x); int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir); int security_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *xp, - const struct flowi *fl); + const struct flowi *fl, + unsigned short family); int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid); void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl); @@ -1679,7 +1680,9 @@ static inline int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_s } static inline int security_xfrm_state_pol_flow_match(struct xfrm_state *x, - struct xfrm_policy *xp, const struct flowi *fl) + struct xfrm_policy *xp, + const struct flowi *fl, + unsigned short family) { return 1; } diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 69520ad3d83b..f90d2f1da44a 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -1020,7 +1020,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x, if (x->km.state == XFRM_STATE_VALID) { if ((x->sel.family && !xfrm_selector_match(&x->sel, fl, x->sel.family)) || - !security_xfrm_state_pol_flow_match(x, pol, fl)) + !security_xfrm_state_pol_flow_match(x, pol, fl, family)) return; if (!*best || @@ -1033,7 +1033,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x, } else if (x->km.state == XFRM_STATE_ERROR || x->km.state == XFRM_STATE_EXPIRED) { if (xfrm_selector_match(&x->sel, fl, x->sel.family) && - security_xfrm_state_pol_flow_match(x, pol, fl)) + security_xfrm_state_pol_flow_match(x, pol, fl, family)) *error = -ESRCH; } } diff --git a/security/security.c b/security/security.c index 70a7ad357bc6..62dd0af7c6bc 100644 --- a/security/security.c +++ b/security/security.c @@ -2391,7 +2391,8 @@ int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir) int security_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *xp, - const struct flowi *fl) + const struct flowi *fl, + unsigned short family) { struct security_hook_list *hp; int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match); @@ -2407,7 +2408,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, */ hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match, list) { - rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl); + rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl, family); break; } return rc; diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h index a0b465316292..36907dd06647 100644 --- a/security/selinux/include/xfrm.h +++ b/security/selinux/include/xfrm.h @@ -26,7 +26,8 @@ int selinux_xfrm_state_delete(struct xfrm_state *x); int selinux_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir); int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *xp, - const struct flowi *fl); + const struct flowi *fl, + unsigned short family); #ifdef CONFIG_SECURITY_NETWORK_XFRM extern atomic_t selinux_xfrm_refcount; diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c index 7314196185d1..5beb30237d3a 100644 --- a/security/selinux/xfrm.c +++ b/security/selinux/xfrm.c @@ -175,7 +175,8 @@ int selinux_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir) */ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *xp, - const struct flowi *fl) + const struct flowi *fl, + unsigned short family) { u32 state_sid;
As pointed out by Herbert in a recent related patch, the LSM hooks should pass the address family in addition to the xfrm flow as the family information is needed to safely access the flow. While this is not technically a problem for the current LSM/SELinux code as it only accesses fields common to all address families, we should still pass the address family so that the LSM hook isn't inherently flawed. An alternate solution could be to simply pass the LSM secid instead of flow, but this introduces the problem of the LSM hook callers sending the wrong secid which would be much worse. Reported-by: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Paul Moore <paul@paul-moore.com> --- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 1 + include/linux/security.h | 7 +++++-- net/xfrm/xfrm_state.c | 4 ++-- security/security.c | 5 +++-- security/selinux/include/xfrm.h | 3 ++- security/selinux/xfrm.c | 3 ++- 7 files changed, 16 insertions(+), 9 deletions(-)