diff mbox series

xfrm: compat: change expression for switch in xfrm_xlate64

Message ID 20230110091450.21696-1-abelova@astralinux.ru
State New
Headers
Series xfrm: compat: change expression for switch in xfrm_xlate64 |

Commit Message

Anastasia Belova Jan. 10, 2023, 9:14 a.m. UTC 
  Compare XFRM_MSG_NEWSPDINFO (value from netlink
configuration messages enum) with nlh_src->nlmsg_type
instead of nlh_src->nlmsg_type - XFRM_MSG_BASE.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 4e9505064f58 ("net/xfrm/compat: Copy xfrm_spdattr_type_t atributes")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 net/xfrm/xfrm_compat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Dmitry Safonov Jan. 10, 2023, 5:32 p.m. UTC | #1 
On 1/10/23 09:14, Anastasia Belova wrote:
> Compare XFRM_MSG_NEWSPDINFO (value from netlink
> configuration messages enum) with nlh_src->nlmsg_type
> instead of nlh_src->nlmsg_type - XFRM_MSG_BASE.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.

Nice find!

Looking at the details:
xfrm_xlate64() is for translating 64-bit kernel-issued message to 32-bit
userspace ABI. The message is XFRM_MSG_NEWSPDINFO and there's a selftest
that checks if the attributes (thresh valued) were correctly translated
in tools/testing/selftests/net/ipsec.c

So, I was interested in how did it go unnoticed?
The switch here is to differ XFRMA_* attributes from XFRMA_SPD_*
attributes, which can be just copied as they are as they occupy the same
space on 64-bit as well as on 32-bit.

enum xfrm_spdattr_type_t {
	XFRMA_SPD_UNSPEC,
	XFRMA_SPD_INFO,
	XFRMA_SPD_HINFO,
	XFRMA_SPD_IPV4_HTHRESH,
	XFRMA_SPD_IPV6_HTHRESH,
	__XFRMA_SPD_MAX
};
attributes went through xfrm_xlate64_attr() instead of just being
copied. That worked in result as
	case XFRMA_UNSPEC:
	case XFRMA_ALG_AUTH:
	case XFRMA_ALG_CRYPT:
	case XFRMA_ALG_COMP:
	case XFRMA_ENCAP:
	case XFRMA_TMPL:
		return xfrm_nla_cpy(dst, src, nla_len(src));
are equal by value (XFRMA_UNSPEC == 0 == XFRMA_SPD_UNSPEC) and so on.
So, in result, even with this typo the code worked.

What about the reverse case, what was being copied by this
XFRM_MSG_NEWSPDINFO case?
XFRM_MSG_NEWSPDINFO == 0x24
So, before this patch (XFRM_MSG_NEWSPDINFO - XFRM_MSG_BASE) == 0x14 ==
XFRM_MSG_DELPOLICY type of messages would fit this switch case.

Luckily enough, kernel doesn't send back XFRM_MSG_DELPOLICY messages to
userspace. That's how it went unnoticed, by unexpectedly still working.


> Fixes: 4e9505064f58 ("net/xfrm/compat: Copy xfrm_spdattr_type_t atributes")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>

Thanks for the patch,
Acked-by: Dmitry Safonov <0x7f454c46@gmail.com>
Tested-by: Dmitry Safonov <0x7f454c46@gmail.com>

> ---
>  net/xfrm/xfrm_compat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
> index a0f62fa02e06..12405aa5bce8 100644
> --- a/net/xfrm/xfrm_compat.c
> +++ b/net/xfrm/xfrm_compat.c
> @@ -302,7 +302,7 @@ static int xfrm_xlate64(struct sk_buff *dst, const struct nlmsghdr *nlh_src)
>  	nla_for_each_attr(nla, attrs, len, remaining) {
>  		int err;
>  
> -		switch (type) {
> +		switch (nlh_src->nlmsg_type) {
>  		case XFRM_MSG_NEWSPDINFO:
>  			err = xfrm_nla_cpy(dst, nla, nla_len(nla));
>  			break;
Steffen Klassert Jan. 16, 2023, 1:01 p.m. UTC | #2 
On Tue, Jan 10, 2023 at 12:14:50PM +0300, Anastasia Belova wrote:
> Compare XFRM_MSG_NEWSPDINFO (value from netlink
> configuration messages enum) with nlh_src->nlmsg_type
> instead of nlh_src->nlmsg_type - XFRM_MSG_BASE.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 4e9505064f58 ("net/xfrm/compat: Copy xfrm_spdattr_type_t atributes")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>

Applied, thanks a lot Anastasia!
diff mbox series

Patch

diff --git a/net/xfrm/xfrm_compat.c b/net/xfrm/xfrm_compat.c
index a0f62fa02e06..12405aa5bce8 100644
--- a/net/xfrm/xfrm_compat.c
+++ b/net/xfrm/xfrm_compat.c
@@ -302,7 +302,7 @@  static int xfrm_xlate64(struct sk_buff *dst, const struct nlmsghdr *nlh_src)
 	nla_for_each_attr(nla, attrs, len, remaining) {
 		int err;
 
-		switch (type) {
+		switch (nlh_src->nlmsg_type) {
 		case XFRM_MSG_NEWSPDINFO:
 			err = xfrm_nla_cpy(dst, nla, nla_len(nla));
 			break;