| Message ID | 20200810165545.31610-1-stefan.bader@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,B,X] xfrm: policy: match with both mark and mask on user interfaces | expand |
On 10/08/2020 17:55, Stefan Bader wrote: > From: Xin Long <lucien.xin@gmail.com> > > In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"), > it would take 'priority' to make a policy unique, and allow duplicated > policies with different 'priority' to be added, which is not expected > by userland, as Tobias reported in strongswan. > > To fix this duplicated policies issue, and also fix the issue in > commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"), > when doing add/del/get/update on user interfaces, this patch is to change > to look up a policy with both mark and mask by doing: > > mark.v == pol->mark.v && mark.m == pol->mark.m > > and leave the check: > > (mark & pol->mark.m) == pol->mark.v > > for tx/rx path only. > > As the userland expects an exact mark and mask match to manage policies. > > v1->v2: > - make xfrm_policy_mark_match inline and fix the changelog as > Tobias suggested. > > Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark") > Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list") > Reported-by: Tobias Brunner <tobias@strongswan.org> > Tested-by: Tobias Brunner <tobias@strongswan.org> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > > BugLink: https://bugs.launchpad.net/bugs/1890796 > > (backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58) > [smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx] > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > > This is my backport (it did compile ok on Bionic but if someone could > double check this is looking sane) of the upstream change. The two > functions primarily touched have one argument less there. It appeared to > apply with some offset to Xenial as well but I have not compiled it > there. > > -Stefan > > > include/net/xfrm.h | 8 +++++--- > net/key/af_key.c | 4 ++-- > net/xfrm/xfrm_policy.c | 29 +++++++++++++---------------- > net/xfrm/xfrm_user.c | 16 ++++++++++------ > 4 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index 83eb033f2cc6..b930c5c8e3d3 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk, > void *); > void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net); > int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl); > -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, > +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, > + const struct xfrm_mark *mark, > u8 type, int dir, > struct xfrm_selector *sel, > struct xfrm_sec_ctx *ctx, int delete, > int *err); > -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir, > - u32 id, int delete, int *err); > +struct xfrm_policy *xfrm_policy_byid(struct net *net, > + const struct xfrm_mark *mark, u8, > + int dir, u32 id, int delete, int *err); > int xfrm_policy_flush(struct net *net, u8 type, bool task_valid); > void xfrm_policy_hash_rebuild(struct net *net); > u32 xfrm_get_acqseq(void); > diff --git a/net/key/af_key.c b/net/key/af_key.c > index d0561c48234e..c8953fe2cfaa 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa > return err; > } > > - xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, > + xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, > pol->sadb_x_policy_dir - 1, &sel, pol_ctx, > 1, &err); > security_xfrm_policy_free(pol_ctx); > @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_ > return -EINVAL; > > delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2); > - xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, > + xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, > dir, pol->sadb_x_policy_id, delete, &err); > if (xp == NULL) > return -ENOENT; > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 3dc1840c81b1..fb9a086395fe 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old, > spin_unlock_bh(&pq->hold_queue.lock); > } > > -static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > - struct xfrm_policy *pol) > +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark, > + struct xfrm_policy *pol) > { > - if (policy->mark.v == pol->mark.v && > - policy->priority == pol->priority) > - return true; > - > - return false; > + return mark->v == pol->mark.v && mark->m == pol->mark.m; > } > > int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == policy->type && > !selector_cmp(&pol->selector, &policy->selector) && > - xfrm_policy_mark_match(policy, pol) && > + xfrm_policy_mark_match(&policy->mark, pol) && > xfrm_sec_ctx_match(pol->security, policy->security) && > !WARN_ON(delpol)) { > if (excl) { > @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > } > EXPORT_SYMBOL(xfrm_policy_insert); > > -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > - int dir, struct xfrm_selector *sel, > - struct xfrm_sec_ctx *ctx, int delete, > - int *err) > +struct xfrm_policy * > +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type, > + int dir, struct xfrm_selector *sel, > + struct xfrm_sec_ctx *ctx, int delete, int *err) > { > struct xfrm_policy *pol, *ret; > struct hlist_head *chain; > @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > ret = NULL; > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == type && > - (mark & pol->mark.m) == pol->mark.v && > + xfrm_policy_mark_match(mark, pol) && > !selector_cmp(sel, &pol->selector) && > xfrm_sec_ctx_match(ctx, pol->security)) { > xfrm_pol_hold(pol); > @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > } > EXPORT_SYMBOL(xfrm_policy_bysel_ctx); > > -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, > - int dir, u32 id, int delete, int *err) > +struct xfrm_policy * > +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type, > + int dir, u32 id, int delete, int *err) > { > struct xfrm_policy *pol, *ret; > struct hlist_head *chain; > @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, > ret = NULL; > hlist_for_each_entry(pol, chain, byidx) { > if (pol->type == type && pol->index == id && > - (mark & pol->mark.m) == pol->mark.v) { > + xfrm_policy_mark_match(mark, pol)) { > xfrm_pol_hold(pol); > if (delete) { > *err = security_xfrm_policy_delete( > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > index 9018cb082e82..b84fe2a71080 100644 > --- a/net/xfrm/xfrm_user.c > +++ b/net/xfrm/xfrm_user.c > @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > struct km_event c; > int delete; > struct xfrm_mark m; > - u32 mark = xfrm_mark_get(attrs, &m); > > p = nlmsg_data(nlh); > delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY; > @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > > + xfrm_mark_get(attrs, &m); > + > if (p->index) > - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err); > + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, > + delete, &err); > else { > struct nlattr *rt = attrs[XFRMA_SEC_CTX]; > struct xfrm_sec_ctx *ctx; > @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > } > - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel, > + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel, > ctx, delete, &err); > security_xfrm_policy_free(ctx); > } > @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > u8 type = XFRM_POLICY_TYPE_MAIN; > int err = -ENOENT; > struct xfrm_mark m; > - u32 mark = xfrm_mark_get(attrs, &m); > > err = copy_from_user_policy_type(&type, attrs); > if (err) > @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > > + xfrm_mark_get(attrs, &m); > + > if (p->index) > - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err); > + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0, > + &err); > else { > struct nlattr *rt = attrs[XFRMA_SEC_CTX]; > struct xfrm_sec_ctx *ctx; > @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > } > - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, > + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, > &p->sel, ctx, 0, &err); > security_xfrm_policy_free(ctx); > } > I've given this a careful eyeballing and it looks good to me, but I'd also appreciate it if somebody gives this a good review as I'm quite heat exhausted and hence may have made a mistake. Acked-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com> On Mon, Aug 10, 2020 at 06:55:45PM +0200, Stefan Bader wrote: > From: Xin Long <lucien.xin@gmail.com> > > In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"), > it would take 'priority' to make a policy unique, and allow duplicated > policies with different 'priority' to be added, which is not expected > by userland, as Tobias reported in strongswan. > > To fix this duplicated policies issue, and also fix the issue in > commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"), > when doing add/del/get/update on user interfaces, this patch is to change > to look up a policy with both mark and mask by doing: > > mark.v == pol->mark.v && mark.m == pol->mark.m > > and leave the check: > > (mark & pol->mark.m) == pol->mark.v > > for tx/rx path only. > > As the userland expects an exact mark and mask match to manage policies. > > v1->v2: > - make xfrm_policy_mark_match inline and fix the changelog as > Tobias suggested. > > Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark") > Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list") > Reported-by: Tobias Brunner <tobias@strongswan.org> > Tested-by: Tobias Brunner <tobias@strongswan.org> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > > BugLink: https://bugs.launchpad.net/bugs/1890796 > > (backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58) > [smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx] > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > > This is my backport (it did compile ok on Bionic but if someone could > double check this is looking sane) of the upstream change. The two > functions primarily touched have one argument less there. It appeared to > apply with some offset to Xenial as well but I have not compiled it > there. > > -Stefan > > > include/net/xfrm.h | 8 +++++--- > net/key/af_key.c | 4 ++-- > net/xfrm/xfrm_policy.c | 29 +++++++++++++---------------- > net/xfrm/xfrm_user.c | 16 ++++++++++------ > 4 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index 83eb033f2cc6..b930c5c8e3d3 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk, > void *); > void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net); > int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl); > -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, > +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, > + const struct xfrm_mark *mark, > u8 type, int dir, > struct xfrm_selector *sel, > struct xfrm_sec_ctx *ctx, int delete, > int *err); > -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir, > - u32 id, int delete, int *err); > +struct xfrm_policy *xfrm_policy_byid(struct net *net, > + const struct xfrm_mark *mark, u8, > + int dir, u32 id, int delete, int *err); > int xfrm_policy_flush(struct net *net, u8 type, bool task_valid); > void xfrm_policy_hash_rebuild(struct net *net); > u32 xfrm_get_acqseq(void); > diff --git a/net/key/af_key.c b/net/key/af_key.c > index d0561c48234e..c8953fe2cfaa 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa > return err; > } > > - xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, > + xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, > pol->sadb_x_policy_dir - 1, &sel, pol_ctx, > 1, &err); > security_xfrm_policy_free(pol_ctx); > @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_ > return -EINVAL; > > delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2); > - xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, > + xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, > dir, pol->sadb_x_policy_id, delete, &err); > if (xp == NULL) > return -ENOENT; > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 3dc1840c81b1..fb9a086395fe 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old, > spin_unlock_bh(&pq->hold_queue.lock); > } > > -static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > - struct xfrm_policy *pol) > +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark, > + struct xfrm_policy *pol) > { > - if (policy->mark.v == pol->mark.v && > - policy->priority == pol->priority) > - return true; > - > - return false; > + return mark->v == pol->mark.v && mark->m == pol->mark.m; > } > > int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == policy->type && > !selector_cmp(&pol->selector, &policy->selector) && > - xfrm_policy_mark_match(policy, pol) && > + xfrm_policy_mark_match(&policy->mark, pol) && > xfrm_sec_ctx_match(pol->security, policy->security) && > !WARN_ON(delpol)) { > if (excl) { > @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > } > EXPORT_SYMBOL(xfrm_policy_insert); > > -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > - int dir, struct xfrm_selector *sel, > - struct xfrm_sec_ctx *ctx, int delete, > - int *err) > +struct xfrm_policy * > +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type, > + int dir, struct xfrm_selector *sel, > + struct xfrm_sec_ctx *ctx, int delete, int *err) > { > struct xfrm_policy *pol, *ret; > struct hlist_head *chain; > @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > ret = NULL; > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == type && > - (mark & pol->mark.m) == pol->mark.v && > + xfrm_policy_mark_match(mark, pol) && > !selector_cmp(sel, &pol->selector) && > xfrm_sec_ctx_match(ctx, pol->security)) { > xfrm_pol_hold(pol); > @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > } > EXPORT_SYMBOL(xfrm_policy_bysel_ctx); > > -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, > - int dir, u32 id, int delete, int *err) > +struct xfrm_policy * > +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type, > + int dir, u32 id, int delete, int *err) > { > struct xfrm_policy *pol, *ret; > struct hlist_head *chain; > @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, > ret = NULL; > hlist_for_each_entry(pol, chain, byidx) { > if (pol->type == type && pol->index == id && > - (mark & pol->mark.m) == pol->mark.v) { > + xfrm_policy_mark_match(mark, pol)) { > xfrm_pol_hold(pol); > if (delete) { > *err = security_xfrm_policy_delete( > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > index 9018cb082e82..b84fe2a71080 100644 > --- a/net/xfrm/xfrm_user.c > +++ b/net/xfrm/xfrm_user.c > @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > struct km_event c; > int delete; > struct xfrm_mark m; > - u32 mark = xfrm_mark_get(attrs, &m); > > p = nlmsg_data(nlh); > delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY; > @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > > + xfrm_mark_get(attrs, &m); > + > if (p->index) > - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err); > + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, > + delete, &err); > else { > struct nlattr *rt = attrs[XFRMA_SEC_CTX]; > struct xfrm_sec_ctx *ctx; > @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > } > - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel, > + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel, > ctx, delete, &err); > security_xfrm_policy_free(ctx); > } > @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > u8 type = XFRM_POLICY_TYPE_MAIN; > int err = -ENOENT; > struct xfrm_mark m; > - u32 mark = xfrm_mark_get(attrs, &m); > > err = copy_from_user_policy_type(&type, attrs); > if (err) > @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > > + xfrm_mark_get(attrs, &m); > + > if (p->index) > - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err); > + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0, > + &err); > else { > struct nlattr *rt = attrs[XFRMA_SEC_CTX]; > struct xfrm_sec_ctx *ctx; > @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > } > - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, > + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, > &p->sel, ctx, 0, &err); > security_xfrm_policy_free(ctx); > } > -- > 2.25.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 10.08.20 18:55, Stefan Bader wrote: > From: Xin Long <lucien.xin@gmail.com> > > In commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"), > it would take 'priority' to make a policy unique, and allow duplicated > policies with different 'priority' to be added, which is not expected > by userland, as Tobias reported in strongswan. > > To fix this duplicated policies issue, and also fix the issue in > commit ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list"), > when doing add/del/get/update on user interfaces, this patch is to change > to look up a policy with both mark and mask by doing: > > mark.v == pol->mark.v && mark.m == pol->mark.m > > and leave the check: > > (mark & pol->mark.m) == pol->mark.v > > for tx/rx path only. > > As the userland expects an exact mark and mask match to manage policies. > > v1->v2: > - make xfrm_policy_mark_match inline and fix the changelog as > Tobias suggested. > > Fixes: 295fae568885 ("xfrm: Allow user space manipulation of SPD mark") > Fixes: ed17b8d377ea ("xfrm: fix a warning in xfrm_policy_insert_list") > Reported-by: Tobias Brunner <tobias@strongswan.org> > Tested-by: Tobias Brunner <tobias@strongswan.org> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > > BugLink: https://bugs.launchpad.net/bugs/1890796 > > (backported from commit 4f47e8ab6ab796b5380f74866fa5287aca4dcc58) > [smb: work around missing if_id parameter and __xfrm_policy_bysel_ctx] > Signed-off-by: Stefan Bader <stefan.bader@canonical.com> > --- > > This is my backport (it did compile ok on Bionic but if someone could > double check this is looking sane) of the upstream change. The two > functions primarily touched have one argument less there. It appeared to > apply with some offset to Xenial as well but I have not compiled it > there. This looks to be applied by Kelsey to bionic,xenial/master-next. Thanks. -Stefan > > -Stefan > > > include/net/xfrm.h | 8 +++++--- > net/key/af_key.c | 4 ++-- > net/xfrm/xfrm_policy.c | 29 +++++++++++++---------------- > net/xfrm/xfrm_user.c | 16 ++++++++++------ > 4 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index 83eb033f2cc6..b930c5c8e3d3 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk, > void *); > void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net); > int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl); > -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, > +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, > + const struct xfrm_mark *mark, > u8 type, int dir, > struct xfrm_selector *sel, > struct xfrm_sec_ctx *ctx, int delete, > int *err); > -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir, > - u32 id, int delete, int *err); > +struct xfrm_policy *xfrm_policy_byid(struct net *net, > + const struct xfrm_mark *mark, u8, > + int dir, u32 id, int delete, int *err); > int xfrm_policy_flush(struct net *net, u8 type, bool task_valid); > void xfrm_policy_hash_rebuild(struct net *net); > u32 xfrm_get_acqseq(void); > diff --git a/net/key/af_key.c b/net/key/af_key.c > index d0561c48234e..c8953fe2cfaa 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa > return err; > } > > - xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, > + xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, > pol->sadb_x_policy_dir - 1, &sel, pol_ctx, > 1, &err); > security_xfrm_policy_free(pol_ctx); > @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_ > return -EINVAL; > > delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2); > - xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, > + xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, > dir, pol->sadb_x_policy_id, delete, &err); > if (xp == NULL) > return -ENOENT; > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 3dc1840c81b1..fb9a086395fe 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old, > spin_unlock_bh(&pq->hold_queue.lock); > } > > -static bool xfrm_policy_mark_match(struct xfrm_policy *policy, > - struct xfrm_policy *pol) > +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark, > + struct xfrm_policy *pol) > { > - if (policy->mark.v == pol->mark.v && > - policy->priority == pol->priority) > - return true; > - > - return false; > + return mark->v == pol->mark.v && mark->m == pol->mark.m; > } > > int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == policy->type && > !selector_cmp(&pol->selector, &policy->selector) && > - xfrm_policy_mark_match(policy, pol) && > + xfrm_policy_mark_match(&policy->mark, pol) && > xfrm_sec_ctx_match(pol->security, policy->security) && > !WARN_ON(delpol)) { > if (excl) { > @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) > } > EXPORT_SYMBOL(xfrm_policy_insert); > > -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > - int dir, struct xfrm_selector *sel, > - struct xfrm_sec_ctx *ctx, int delete, > - int *err) > +struct xfrm_policy * > +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type, > + int dir, struct xfrm_selector *sel, > + struct xfrm_sec_ctx *ctx, int delete, int *err) > { > struct xfrm_policy *pol, *ret; > struct hlist_head *chain; > @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > ret = NULL; > hlist_for_each_entry(pol, chain, bydst) { > if (pol->type == type && > - (mark & pol->mark.m) == pol->mark.v && > + xfrm_policy_mark_match(mark, pol) && > !selector_cmp(sel, &pol->selector) && > xfrm_sec_ctx_match(ctx, pol->security)) { > xfrm_pol_hold(pol); > @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, > } > EXPORT_SYMBOL(xfrm_policy_bysel_ctx); > > -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, > - int dir, u32 id, int delete, int *err) > +struct xfrm_policy * > +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type, > + int dir, u32 id, int delete, int *err) > { > struct xfrm_policy *pol, *ret; > struct hlist_head *chain; > @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, > ret = NULL; > hlist_for_each_entry(pol, chain, byidx) { > if (pol->type == type && pol->index == id && > - (mark & pol->mark.m) == pol->mark.v) { > + xfrm_policy_mark_match(mark, pol)) { > xfrm_pol_hold(pol); > if (delete) { > *err = security_xfrm_policy_delete( > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > index 9018cb082e82..b84fe2a71080 100644 > --- a/net/xfrm/xfrm_user.c > +++ b/net/xfrm/xfrm_user.c > @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > struct km_event c; > int delete; > struct xfrm_mark m; > - u32 mark = xfrm_mark_get(attrs, &m); > > p = nlmsg_data(nlh); > delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY; > @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > > + xfrm_mark_get(attrs, &m); > + > if (p->index) > - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err); > + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, > + delete, &err); > else { > struct nlattr *rt = attrs[XFRMA_SEC_CTX]; > struct xfrm_sec_ctx *ctx; > @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > } > - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel, > + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel, > ctx, delete, &err); > security_xfrm_policy_free(ctx); > } > @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > u8 type = XFRM_POLICY_TYPE_MAIN; > int err = -ENOENT; > struct xfrm_mark m; > - u32 mark = xfrm_mark_get(attrs, &m); > > err = copy_from_user_policy_type(&type, attrs); > if (err) > @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > > + xfrm_mark_get(attrs, &m); > + > if (p->index) > - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err); > + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0, > + &err); > else { > struct nlattr *rt = attrs[XFRMA_SEC_CTX]; > struct xfrm_sec_ctx *ctx; > @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, > if (err) > return err; > } > - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, > + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, > &p->sel, ctx, 0, &err); > security_xfrm_policy_free(ctx); > } >
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 83eb033f2cc6..b930c5c8e3d3 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1674,13 +1674,15 @@ int xfrm_policy_walk(struct net *net, struct xfrm_policy_walk *walk, void *); void xfrm_policy_walk_done(struct xfrm_policy_walk *walk, struct net *net); int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl); -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, +struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, + const struct xfrm_mark *mark, u8 type, int dir, struct xfrm_selector *sel, struct xfrm_sec_ctx *ctx, int delete, int *err); -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir, - u32 id, int delete, int *err); +struct xfrm_policy *xfrm_policy_byid(struct net *net, + const struct xfrm_mark *mark, u8, + int dir, u32 id, int delete, int *err); int xfrm_policy_flush(struct net *net, u8 type, bool task_valid); void xfrm_policy_hash_rebuild(struct net *net); u32 xfrm_get_acqseq(void); diff --git a/net/key/af_key.c b/net/key/af_key.c index d0561c48234e..c8953fe2cfaa 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -2406,7 +2406,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa return err; } - xp = xfrm_policy_bysel_ctx(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, + xp = xfrm_policy_bysel_ctx(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, pol->sadb_x_policy_dir - 1, &sel, pol_ctx, 1, &err); security_xfrm_policy_free(pol_ctx); @@ -2657,7 +2657,7 @@ static int pfkey_spdget(struct sock *sk, struct sk_buff *skb, const struct sadb_ return -EINVAL; delete = (hdr->sadb_msg_type == SADB_X_SPDDELETE2); - xp = xfrm_policy_byid(net, DUMMY_MARK, XFRM_POLICY_TYPE_MAIN, + xp = xfrm_policy_byid(net, &dummy_mark, XFRM_POLICY_TYPE_MAIN, dir, pol->sadb_x_policy_id, delete, &err); if (xp == NULL) return -ENOENT; diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 3dc1840c81b1..fb9a086395fe 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -718,14 +718,10 @@ static void xfrm_policy_requeue(struct xfrm_policy *old, spin_unlock_bh(&pq->hold_queue.lock); } -static bool xfrm_policy_mark_match(struct xfrm_policy *policy, - struct xfrm_policy *pol) +static inline bool xfrm_policy_mark_match(const struct xfrm_mark *mark, + struct xfrm_policy *pol) { - if (policy->mark.v == pol->mark.v && - policy->priority == pol->priority) - return true; - - return false; + return mark->v == pol->mark.v && mark->m == pol->mark.m; } int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) @@ -743,7 +739,7 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) hlist_for_each_entry(pol, chain, bydst) { if (pol->type == policy->type && !selector_cmp(&pol->selector, &policy->selector) && - xfrm_policy_mark_match(policy, pol) && + xfrm_policy_mark_match(&policy->mark, pol) && xfrm_sec_ctx_match(pol->security, policy->security) && !WARN_ON(delpol)) { if (excl) { @@ -793,10 +789,10 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl) } EXPORT_SYMBOL(xfrm_policy_insert); -struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, - int dir, struct xfrm_selector *sel, - struct xfrm_sec_ctx *ctx, int delete, - int *err) +struct xfrm_policy * +xfrm_policy_bysel_ctx(struct net *net, const struct xfrm_mark *mark, u8 type, + int dir, struct xfrm_selector *sel, + struct xfrm_sec_ctx *ctx, int delete, int *err) { struct xfrm_policy *pol, *ret; struct hlist_head *chain; @@ -807,7 +803,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, ret = NULL; hlist_for_each_entry(pol, chain, bydst) { if (pol->type == type && - (mark & pol->mark.m) == pol->mark.v && + xfrm_policy_mark_match(mark, pol) && !selector_cmp(sel, &pol->selector) && xfrm_sec_ctx_match(ctx, pol->security)) { xfrm_pol_hold(pol); @@ -832,8 +828,9 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(struct net *net, u32 mark, u8 type, } EXPORT_SYMBOL(xfrm_policy_bysel_ctx); -struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, - int dir, u32 id, int delete, int *err) +struct xfrm_policy * +xfrm_policy_byid(struct net *net, const struct xfrm_mark *mark, u8 type, + int dir, u32 id, int delete, int *err) { struct xfrm_policy *pol, *ret; struct hlist_head *chain; @@ -848,7 +845,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8 type, ret = NULL; hlist_for_each_entry(pol, chain, byidx) { if (pol->type == type && pol->index == id && - (mark & pol->mark.m) == pol->mark.v) { + xfrm_policy_mark_match(mark, pol)) { xfrm_pol_hold(pol); if (delete) { *err = security_xfrm_policy_delete( diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 9018cb082e82..b84fe2a71080 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -1820,7 +1820,6 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, struct km_event c; int delete; struct xfrm_mark m; - u32 mark = xfrm_mark_get(attrs, &m); p = nlmsg_data(nlh); delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY; @@ -1833,8 +1832,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, if (err) return err; + xfrm_mark_get(attrs, &m); + if (p->index) - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, delete, &err); + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, + delete, &err); else { struct nlattr *rt = attrs[XFRMA_SEC_CTX]; struct xfrm_sec_ctx *ctx; @@ -1851,7 +1853,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh, if (err) return err; } - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, &p->sel, + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel, ctx, delete, &err); security_xfrm_policy_free(ctx); } @@ -2115,7 +2117,6 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, u8 type = XFRM_POLICY_TYPE_MAIN; int err = -ENOENT; struct xfrm_mark m; - u32 mark = xfrm_mark_get(attrs, &m); err = copy_from_user_policy_type(&type, attrs); if (err) @@ -2125,8 +2126,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, if (err) return err; + xfrm_mark_get(attrs, &m); + if (p->index) - xp = xfrm_policy_byid(net, mark, type, p->dir, p->index, 0, &err); + xp = xfrm_policy_byid(net, &m, type, p->dir, p->index, 0, + &err); else { struct nlattr *rt = attrs[XFRMA_SEC_CTX]; struct xfrm_sec_ctx *ctx; @@ -2143,7 +2147,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh, if (err) return err; } - xp = xfrm_policy_bysel_ctx(net, mark, type, p->dir, + xp = xfrm_policy_bysel_ctx(net, &m, type, p->dir, &p->sel, ctx, 0, &err); security_xfrm_policy_free(ctx); }