netdev - Re: [PATCH v3] net: xfrm: Amend XFRMA_SEC_CTX nla

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZJ6L+chlwapjZO+A@gauss3.secunet.de>
Date: Fri, 30 Jun 2023 10:02:01 +0200
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Lin Ma <linma@....edu.cn>
CC: <herbert@...dor.apana.org.au>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, <tgraf@...g.ch>, <simon.horman@...igine.com>
Subject: Re: [PATCH v3] net: xfrm: Amend XFRMA_SEC_CTX nla_policy structure

On Fri, Jun 30, 2023 at 09:22:41AM +0800, Lin Ma wrote:
> According to all consumers code of attrs[XFRMA_SEC_CTX], like
> 
> * verify_sec_ctx_len(), convert to xfrm_user_sec_ctx*
> * xfrm_state_construct(), call security_xfrm_state_alloc whose prototype
> is int security_xfrm_state_alloc(.., struct xfrm_user_sec_ctx *sec_ctx);
> * copy_from_user_sec_ctx(), convert to xfrm_user_sec_ctx *
> ...
> 
> It seems that the expected parsing result for XFRMA_SEC_CTX should be
> structure xfrm_user_sec_ctx, and the current xfrm_sec_ctx is confusing
> and misleading (Luckily, they happen to have same size 8 bytes).
> 
> This commit amend the policy structure to xfrm_user_sec_ctx to avoid
> ambiguity.
> 
> Fixes: cf5cb79f6946 ("[XFRM] netlink: Establish an attribute policy")
> Signed-off-by: Lin Ma <linma@....edu.cn>
> ---
> V1 -> V2: also amend compat_policy XFRMA_SEC_CTX
> V2 -> V3: fix typo
> 
>  net/xfrm/xfrm_compat.c | 2 +-
>  net/xfrm/xfrm_user.c   | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> index 8cbf45a8bcdc..655fe4ff8621 100644
> --- a/net/xfrm/xfrm_compat.c
> +++ b/net/xfrm/xfrm_compat.c
> @@ -108,7 +108,7 @@ static const struct nla_policy compat_policy[XFRMA_MAX+1] = {
>  	[XFRMA_ALG_COMP]	= { .len = sizeof(struct xfrm_algo) },
>  	[XFRMA_ENCAP]		= { .len = sizeof(struct xfrm_encap_tmpl) },
>  	[XFRMA_TMPL]		= { .len = sizeof(struct xfrm_user_tmpl) },
> -	[XFRMA_SEC_CTX]		= { .len = sizeof(struct xfrm_sec_ctx) },
> +	[XFRMA_SEC_CTX]		= { .len = sizeof(struct xfrm_user_sec_ctx) },
>  	[XFRMA_LTIME_VAL]	= { .len = sizeof(struct xfrm_lifetime_cur) },
>  	[XFRMA_REPLAY_VAL]	= { .len = sizeof(struct xfrm_replay_state) },
>  	[XFRMA_REPLAY_THRESH]	= { .type = NLA_U32 },
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index c34a2a06ca94..0c997a114d13 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -3015,7 +3015,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
>  	[XFRMA_ALG_COMP]	= { .len = sizeof(struct xfrm_algo) },
>  	[XFRMA_ENCAP]		= { .len = sizeof(struct xfrm_encap_tmpl) },
>  	[XFRMA_TMPL]		= { .len = sizeof(struct xfrm_user_tmpl) },
> -	[XFRMA_SEC_CTX]		= { .len = sizeof(struct xfrm_sec_ctx) },
> +	[XFRMA_SEC_CTX]		= { .len = sizeof(struct xfrm_user_sec_ctx) },
>  	[XFRMA_LTIME_VAL]	= { .len = sizeof(struct xfrm_lifetime_cur) },
>  	[XFRMA_REPLAY_VAL]	= { .len = sizeof(struct xfrm_replay_state) },
>  	[XFRMA_REPLAY_THRESH]	= { .type = NLA_U32 },
> @@ -3035,6 +3035,7 @@ const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
>  	[XFRMA_SET_MARK]	= { .type = NLA_U32 },
>  	[XFRMA_SET_MARK_MASK]	= { .type = NLA_U32 },
>  	[XFRMA_IF_ID]		= { .type = NLA_U32 },
> +	[XFRMA_MTIMER_THRESH]	= { .type = NLA_U32 },

The above line is apparently missing, but does it really belong to this
patch? At least it is not mentioned in the commit message.