netdev - Re: [PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct xfrm_usersa

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 9 Jan 2014 18:58:58 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Fan Du <fan.du@...driver.com>
CC:	<steffen.klassert@...unet.com>, <davem@...emloft.net>,
	<stephen@...workplumber.org>, <dev@...ts.strongswan.org>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 2/2] include/uapi/linux/xfrm.h: Pack struct
 xfrm_usersa_info

On Thu, 2014-01-09 at 16:24 +0800, Fan Du wrote:
> 
> On 2014年01月09日 04:33, Ben Hutchings wrote:
> > On Tue, 2014-01-07 at 14:48 +0800, Fan Du wrote:
> >> Otherwise 64bits kernel has sizeof(struct xfrm_usersa_info) 224 bytes,
> >> while 32bits compiled iproute2 see the same structure as 220 bytes, which
> >> leading deficit xfrm sa, in turn broken IPsec connectivity.
> >>
> >> Fix this by packing the structure.
> >>
> >> Signed-off-by: Fan Du<fan.du@...driver.com>
> >> ---
> >>   include/uapi/linux/xfrm.h |    2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> >> index 470bfae..61460c4 100644
> >> --- a/include/uapi/linux/xfrm.h
> >> +++ b/include/uapi/linux/xfrm.h
> >> @@ -366,7 +366,7 @@ struct xfrm_usersa_info {
> >>   #define XFRM_STATE_AF_UNSPEC	32
> >>   #define XFRM_STATE_ALIGN4	64
> >>   #define XFRM_STATE_ESN		128
> >> -};
> >> +} __attribute__((packed));
> >>
> >>   #define XFRM_SA_XFLAG_DONT_ENCAP_DSCP	1
> >>
> >
> > That change will make access to the structure very slow on some
> > architectures, and I suspect it will cause other compatibility problems.
> >
> > I think the right thing to do is to reduce the minimum length of the
> > structure in the netlink policy so that padding at the end is not
> > required.
> 
> Could you please be more specific about this? Thanks.
[...]

Each netlink message and attribute has a specified minimum size, and
messages are rejected if the size provided by the sender is too small.
The minimum size is normally the same as the structure size, but doesn't
have to be.

Something like this might work:

--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2266,8 +2266,15 @@ static int xfrm_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 
 #define XMSGSIZE(type) sizeof(struct type)
 
+/* Padding at the end of struct xfrm_usersa_info differs between
+ * architectures so for 32/64-bit compat we don't require padding
+ */
+#define XMSGSIZE_XFRM_USERSA_INFO				\
+	(offsetof(struct xfrm_usersa_info, flags) +		\
+	 sizeof(((struct xfrm_usersa_info *)NULL)->flags))
+
 static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
-	[XFRM_MSG_NEWSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_info),
+	[XFRM_MSG_NEWSA       - XFRM_MSG_BASE] = XMSGSIZE_XFRM_USERSA_INFO,
 	[XFRM_MSG_DELSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_id),
 	[XFRM_MSG_GETSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_id),
 	[XFRM_MSG_NEWPOLICY   - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_info),
@@ -2277,7 +2284,7 @@ static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
 	[XFRM_MSG_ACQUIRE     - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_acquire),
 	[XFRM_MSG_EXPIRE      - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_expire),
 	[XFRM_MSG_UPDPOLICY   - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_info),
-	[XFRM_MSG_UPDSA       - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_info),
+	[XFRM_MSG_UPDSA       - XFRM_MSG_BASE] = XMSGSIZE_XFRM_USERSA_INFO,
 	[XFRM_MSG_POLEXPIRE   - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_polexpire),
 	[XFRM_MSG_FLUSHSA     - XFRM_MSG_BASE] = XMSGSIZE(xfrm_usersa_flush),
 	[XFRM_MSG_FLUSHPOLICY - XFRM_MSG_BASE] = 0,
@@ -2292,7 +2299,7 @@ static const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
 #undef XMSGSIZE
 
 static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
-	[XFRMA_SA]		= { .len = sizeof(struct xfrm_usersa_info)},
+	[XFRMA_SA]		= { .len = XMSGSIZE_XFRM_USERSA_INFO },
 	[XFRMA_POLICY]		= { .len = sizeof(struct xfrm_userpolicy_info)},
 	[XFRMA_LASTUSED]	= { .type = NLA_U64},
 	[XFRMA_ALG_AUTH_TRUNC]	= { .len = sizeof(struct xfrm_algo_auth)},
---

But beware that if the structure is copied in and then copied back out
to userland then the padding will need to be explicitly cleared.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html