ACK: [SRU B, X] xfrm: policy: match with both mark and mask on user interfaces
Marcelo Henrique Cerri
marcelo.cerri at canonical.com
Mon Aug 10 17:58:48 UTC 2020
Acked-by: Marcelo Henrique Cerri <marcelo.cerri at canonical.com>
On Mon, Aug 10, 2020 at 06:55:45PM +0200, Stefan Bader wrote:
> From: Xin Long <lucien.xin at 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 at strongswan.org>
> Tested-by: Tobias Brunner <tobias at strongswan.org>
> Signed-off-by: Xin Long <lucien.xin at gmail.com>
> Signed-off-by: Steffen Klassert <steffen.klassert at 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 at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
--
Regards,
Marcelo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200810/1f25233a/attachment-0001.sig>
More information about the kernel-team
mailing list