* [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
@ 2014-02-19 9:12 Fan Du
2014-02-20 9:59 ` Steffen Klassert
0 siblings, 1 reply; 7+ messages in thread
From: Fan Du @ 2014-02-19 9:12 UTC (permalink / raw)
To: steffen.klassert; +Cc: davem, netdev
When trying to setup IPsec configuration on a 64bits host with
iproute2(32bits compiled), the intended xfrm policy and sa is
either deficit or wrong when kernel trying to parse user land
information.
Add compat support for 32bits compiled ip command for 64bits host.
Signed-off-by: Fan Du <fan.du@windriver.com>
---
include/uapi/linux/xfrm.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
net/xfrm/xfrm_user.c | 16 +++++++++++++++-
2 files changed, 59 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index a8cd6a4..8d2fa8d 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -506,4 +506,48 @@ enum xfrm_nlgroups {
};
#define XFRMNLGRP_MAX (__XFRMNLGRP_MAX - 1)
+#ifdef CONFIG_COMPAT
+
+struct compat_xfrm_userpolicy_info {
+ struct xfrm_selector sel;
+ struct xfrm_lifetime_cfg lft;
+ struct xfrm_lifetime_cur curlft;
+ __u32 priority;
+ __u32 index;
+ __u8 dir;
+ __u8 action;
+#define XFRM_POLICY_ALLOW 0
+#define XFRM_POLICY_BLOCK 1
+ __u8 flags;
+#define XFRM_POLICY_LOCALOK 1 /* Allow user to override global policy */
+ /* Automatically expand selector to include matching ICMP payloads. */
+#define XFRM_POLICY_ICMP 2
+ __u8 share;
+} __attribute__((packed));
+
+struct compat_xfrm_usersa_info {
+ struct xfrm_selector sel;
+ struct xfrm_id id;
+ xfrm_address_t saddr;
+ struct xfrm_lifetime_cfg lft;
+ struct xfrm_lifetime_cur curlft;
+ struct xfrm_stats stats;
+ __u32 seq;
+ __u32 reqid;
+ __u16 family;
+ __u8 mode; /* XFRM_MODE_xxx */
+ __u8 replay_window;
+ __u8 flags;
+#define XFRM_STATE_NOECN 1
+#define XFRM_STATE_DECAP_DSCP 2
+#define XFRM_STATE_NOPMTUDISC 4
+#define XFRM_STATE_WILDRECV 8
+#define XFRM_STATE_ICMP 16
+#define XFRM_STATE_AF_UNSPEC 32
+#define XFRM_STATE_ALIGN4 64
+#define XFRM_STATE_ESN 128
+} __attribute__((packed));
+
+#endif
+
#endif /* _LINUX_XFRM_H */
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 1ae3ec7..8bfbd95 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2281,6 +2281,12 @@ static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
[XFRM_MSG_GETSPDINFO - XFRM_MSG_BASE] = sizeof(u32),
};
+static const int compat_xfrm_msg_min[XFRM_NR_MSGTYPES] = {
+ [XFRM_MSG_NEWSA - XFRM_MSG_BASE] = XMSGSIZE(compat_xfrm_usersa_info),
+ [XFRM_MSG_NEWPOLICY - XFRM_MSG_BASE] = XMSGSIZE(compat_xfrm_userpolicy_info),
+ [XFRM_MSG_UPDPOLICY - XFRM_MSG_BASE] = XMSGSIZE(compat_xfrm_userpolicy_info),
+};
+
#undef XMSGSIZE
static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
@@ -2346,6 +2352,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
struct nlattr *attrs[XFRMA_MAX+1];
const struct xfrm_link *link;
int type, err;
+ int len;
type = nlh->nlmsg_type;
if (type > XFRM_MSG_MAX)
@@ -2373,7 +2380,14 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
}
}
- err = nlmsg_parse(nlh, xfrm_msg_min[type], attrs, XFRMA_MAX,
+#ifdef CONFIG_COMPAT
+ if (is_compat_task())
+ len = compat_xfrm_msg_min[type];
+ else
+#endif
+ len = xfrm_msg_min[type];
+
+ err = nlmsg_parse(nlh, len, attrs, XFRMA_MAX,
xfrma_policy);
if (err < 0)
return err;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
2014-02-19 9:12 [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host Fan Du
@ 2014-02-20 9:59 ` Steffen Klassert
2014-02-25 6:41 ` Fan Du
0 siblings, 1 reply; 7+ messages in thread
From: Steffen Klassert @ 2014-02-20 9:59 UTC (permalink / raw)
To: Fan Du; +Cc: davem, netdev
On Wed, Feb 19, 2014 at 05:12:56PM +0800, Fan Du wrote:
>
> +#ifdef CONFIG_COMPAT
> +
> +struct compat_xfrm_userpolicy_info {
> + struct xfrm_selector sel;
> + struct xfrm_lifetime_cfg lft;
> + struct xfrm_lifetime_cur curlft;
> + __u32 priority;
> + __u32 index;
> + __u8 dir;
> + __u8 action;
> +#define XFRM_POLICY_ALLOW 0
> +#define XFRM_POLICY_BLOCK 1
> + __u8 flags;
> +#define XFRM_POLICY_LOCALOK 1 /* Allow user to override global policy */
> + /* Automatically expand selector to include matching ICMP payloads. */
> +#define XFRM_POLICY_ICMP 2
> + __u8 share;
> +} __attribute__((packed));
> +
> +struct compat_xfrm_usersa_info {
> + struct xfrm_selector sel;
> + struct xfrm_id id;
> + xfrm_address_t saddr;
> + struct xfrm_lifetime_cfg lft;
> + struct xfrm_lifetime_cur curlft;
> + struct xfrm_stats stats;
> + __u32 seq;
> + __u32 reqid;
> + __u16 family;
> + __u8 mode; /* XFRM_MODE_xxx */
> + __u8 replay_window;
> + __u8 flags;
> +#define XFRM_STATE_NOECN 1
> +#define XFRM_STATE_DECAP_DSCP 2
> +#define XFRM_STATE_NOPMTUDISC 4
> +#define XFRM_STATE_WILDRECV 8
> +#define XFRM_STATE_ICMP 16
> +#define XFRM_STATE_AF_UNSPEC 32
> +#define XFRM_STATE_ALIGN4 64
> +#define XFRM_STATE_ESN 128
> +} __attribute__((packed));
> +
You define two structures that you actually don't use,
all you do is checking their size.
The only reason why this works is because these two
structures differ only on some padding bytes at the
end. If we want to support 32 bit ipsec tools on
64 bit kernels, we need a complete compat layer
for all the userspace exported structures.
A lot of userspace exported structures differ not only
on some padding bytes at the end.
For example the layout of xfrm_userspi_info:
on 32 bit:
struct xfrm_userspi_info {
struct xfrm_usersa_info info; /* 0 220 */
/* XXX last struct has 3 bytes of padding */
/* --- cacheline 3 boundary (192 bytes) was 28 bytes ago --- */
__u32 min; /* 220 4 */
__u32 max; /* 224 4 */
/* size: 228, cachelines: 4, members: 3 */
/* paddings: 1, sum paddings: 3 */
/* last cacheline: 36 bytes */
};
on 64 bit:
struct xfrm_userspi_info {
struct xfrm_usersa_info info; /* 0 224 */
/* XXX last struct has 7 bytes of padding */
/* --- cacheline 3 boundary (192 bytes) was 32 bytes ago --- */
__u32 min; /* 224 4 */
__u32 max; /* 228 4 */
/* size: 232, cachelines: 4, members: 3 */
/* paddings: 1, sum paddings: 7 */
/* last cacheline: 40 bytes */
};
So the 'min' field has an offest of 220 bytes on 32 bit and an offset
of 224 bytes on 64 bit. We would need a compatability layer like we
have it for system calls to map this correct.
For now I think we should just refuse to do anything if someone tries
to configure ipsec with 32 bit tools on a 64 bit machine.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
2014-02-20 9:59 ` Steffen Klassert
@ 2014-02-25 6:41 ` Fan Du
2014-02-25 11:53 ` Steffen Klassert
2014-02-25 17:15 ` Ben Hutchings
0 siblings, 2 replies; 7+ messages in thread
From: Fan Du @ 2014-02-25 6:41 UTC (permalink / raw)
To: Steffen Klassert; +Cc: davem, netdev
On 2014年02月20日 17:59, Steffen Klassert wrote:
> For now I think we should just refuse to do anything if someone tries
> to configure ipsec with 32 bit tools on a 64 bit machine.
I'm fine with your point, and it would be a good choice to inform user about
this behavior other than just creating non-working SA and SP for user.
From 873812ec0fe8738f476de58a217e58ec47665180 Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Tue, 25 Feb 2014 14:34:41 +0800
Subject: [PATCH net-next] xfrm: Do not parse 32bits compiled xfrm netlink msg on
64bits host
structure like xfrm_usersa_info or xfrm_userpolicy_info has different sizeof
when compiled as 32bits and 64bits due to not appending pack attribute in
their definition. This will result in broken SA and SP information when user
trying to configure them through netlink interface.
Before forging a compatibility layer like we have it for system calls to map
this correct. Inform user land about this situation instead of keeping silent,
then the upper test scripts could behave accordingly.
Signed-off-by: Fan Du <fan.du@windriver.com>
---
net/xfrm/xfrm_user.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 1ae3ec7..0249712 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2347,6 +2347,10 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
const struct xfrm_link *link;
int type, err;
+#ifdef CONFIG_COMPAT
+ if (is_compat_task())
+ return -EPERM;
+#endif
type = nlh->nlmsg_type;
if (type > XFRM_MSG_MAX)
return -EINVAL;
--
1.7.9.5
--
浮沉随浪只记今朝笑
--fan
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
2014-02-25 6:41 ` Fan Du
@ 2014-02-25 11:53 ` Steffen Klassert
2014-02-25 17:15 ` Ben Hutchings
1 sibling, 0 replies; 7+ messages in thread
From: Steffen Klassert @ 2014-02-25 11:53 UTC (permalink / raw)
To: Fan Du; +Cc: davem, netdev
On Tue, Feb 25, 2014 at 02:41:11PM +0800, Fan Du wrote:
>
>
> On 2014年02月20日 17:59, Steffen Klassert wrote:
> >For now I think we should just refuse to do anything if someone tries
> >to configure ipsec with 32 bit tools on a 64 bit machine.
>
> I'm fine with your point, and it would be a good choice to inform user about
> this behavior other than just creating non-working SA and SP for user.
>
>
> From 873812ec0fe8738f476de58a217e58ec47665180 Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Tue, 25 Feb 2014 14:34:41 +0800
> Subject: [PATCH net-next] xfrm: Do not parse 32bits compiled xfrm netlink msg on
> 64bits host
>
> structure like xfrm_usersa_info or xfrm_userpolicy_info has different sizeof
> when compiled as 32bits and 64bits due to not appending pack attribute in
> their definition. This will result in broken SA and SP information when user
> trying to configure them through netlink interface.
>
> Before forging a compatibility layer like we have it for system calls to map
> this correct. Inform user land about this situation instead of keeping silent,
> then the upper test scripts could behave accordingly.
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
I'm ok with your patch, but it does not apply to ipsec-next.
Please rebase to ipsec-next current and resend.
Thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
2014-02-25 6:41 ` Fan Du
2014-02-25 11:53 ` Steffen Klassert
@ 2014-02-25 17:15 ` Ben Hutchings
2014-02-25 19:16 ` Florian Westphal
1 sibling, 1 reply; 7+ messages in thread
From: Ben Hutchings @ 2014-02-25 17:15 UTC (permalink / raw)
To: Fan Du; +Cc: Steffen Klassert, davem, netdev
[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]
On Tue, 2014-02-25 at 14:41 +0800, Fan Du wrote:
>
> On 2014年02月20日 17:59, Steffen Klassert wrote:
> > For now I think we should just refuse to do anything if someone tries
> > to configure ipsec with 32 bit tools on a 64 bit machine.
>
> I'm fine with your point, and it would be a good choice to inform user about
> this behavior other than just creating non-working SA and SP for user.
>
>
> From 873812ec0fe8738f476de58a217e58ec47665180 Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Tue, 25 Feb 2014 14:34:41 +0800
> Subject: [PATCH net-next] xfrm: Do not parse 32bits compiled xfrm netlink msg on
> 64bits host
>
> structure like xfrm_usersa_info or xfrm_userpolicy_info has different sizeof
> when compiled as 32bits and 64bits due to not appending pack attribute in
> their definition. This will result in broken SA and SP information when user
> trying to configure them through netlink interface.
>
> Before forging a compatibility layer like we have it for system calls to map
> this correct. Inform user land about this situation instead of keeping silent,
> then the upper test scripts could behave accordingly.
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
> net/xfrm/xfrm_user.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 1ae3ec7..0249712 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2347,6 +2347,10 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> const struct xfrm_link *link;
> int type, err;
>
> +#ifdef CONFIG_COMPAT
> + if (is_compat_task())
> + return -EPERM;
I think this needs a log message, as it is not at all obvious that EPERM
means you ran a binary from the 'wrong' architecture.
Ben.
> +#endif
> type = nlh->nlmsg_type;
> if (type > XFRM_MSG_MAX)
> return -EINVAL;
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert Einstein
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
2014-02-25 17:15 ` Ben Hutchings
@ 2014-02-25 19:16 ` Florian Westphal
2014-02-27 23:52 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Florian Westphal @ 2014-02-25 19:16 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Fan Du, Steffen Klassert, davem, netdev
Ben Hutchings <ben@decadent.org.uk> wrote:
> > On 2014年02月20日 17:59, Steffen Klassert wrote:
> > > For now I think we should just refuse to do anything if someone tries
> > > to configure ipsec with 32 bit tools on a 64 bit machine.
> >
> > I'm fine with your point, and it would be a good choice to inform user about
> > this behavior other than just creating non-working SA and SP for user.
It might be better to try to revive
http://thread.gmane.org/gmane.linux.network/157118/focus=157122
which adds full compat support.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host
2014-02-25 19:16 ` Florian Westphal
@ 2014-02-27 23:52 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2014-02-27 23:52 UTC (permalink / raw)
To: fw; +Cc: ben, fan.du, steffen.klassert, netdev
From: Florian Westphal <fw@strlen.de>
Date: Tue, 25 Feb 2014 20:16:02 +0100
> Ben Hutchings <ben@decadent.org.uk> wrote:
>> > On 2014年02月20日 17:59, Steffen Klassert wrote:
>> > > For now I think we should just refuse to do anything if someone tries
>> > > to configure ipsec with 32 bit tools on a 64 bit machine.
>> >
>> > I'm fine with your point, and it would be a good choice to inform user about
>> > this behavior other than just creating non-working SA and SP for user.
>
> It might be better to try to revive
> http://thread.gmane.org/gmane.linux.network/157118/focus=157122
>
> which adds full compat support.
Agreed.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-02-27 23:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-19 9:12 [PATCH net-next] xfrm: Correctly parse netlink msg from 32bits ip command on 64bits host Fan Du
2014-02-20 9:59 ` Steffen Klassert
2014-02-25 6:41 ` Fan Du
2014-02-25 11:53 ` Steffen Klassert
2014-02-25 17:15 ` Ben Hutchings
2014-02-25 19:16 ` Florian Westphal
2014-02-27 23:52 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.