| Message ID | 20230110091450.21696-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm: compat: change expression for switch in xfrm_xlate64 | expand |
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;
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 --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;
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(-)