netdev - RE: [PATCHv3, ipsec-next] xfrm: Do not parse 32bits compiled xfrm netlink msg on 64bits host

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: <063D6719AE5E284EB5DD2968C1650D6D1CAD56B0@AcuExch.aculab.com>
Date:	Thu, 29 Jan 2015 13:56:50 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	"'nicolas.dichtel@...nd.com'" <nicolas.dichtel@...nd.com>,
	Fan Du <fan.du@...el.com>,
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>
CC:	"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"fengyuleidian0615@...il.com" <fengyuleidian0615@...il.com>
Subject: RE: [PATCHv3, ipsec-next] xfrm: Do not parse 32bits compiled xfrm
 netlink msg on 64bits host

From: Nicolas Dichtel
> Le 27/01/2015 10:00, Fan Du a crit :
> > 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.
> >
> > Inform user land about this situation instead of keeping
> > silent, the upper test scripts would behave accordingly.
> >
> > Quotes from: http://marc.info/?l=linux-netdev&m=142226348715503&w=2
> >>
> >> Before a clean solution show up, I think it's better to warn user in some way
> >> like http://patchwork.ozlabs.org/patch/323842/ did. Otherwise, many people
> >> who stuck there will always spend time and try to fix this issue in whatever way.
> >
> > Yes, this is the first thing we should do. I'm willing to accept a patch
> >
> > Signed-off-by: Fan Du <fan.du@...el.com>
> A way to solve this problem was to provide to userland a xfrm compat header
> file, which match the ABI of the kernel. Something like:
> 
> #include <linux/xfrm.h>
> 
> #define xfrm_usersa_info xfrm_usersa_info_64
> #define xfrm_usersa_info_compat xfrm_usersa_info
> struct xfrm_usersa_info_compat {
> 	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;
> 	__u8				replay_window;
> 	__u8				flags;
> 	__u8				hole1;
> 	__u32				hole2;
> };
> 
> The point I try to make is that patching userland apps allows to use xfrm on a
> 32bits userland / 64bits kernel.
> 
> If I understand well your patch, it will not be possible anymore, all messages
> will be rejected. And this may break existing apps.

Probably OTT in this case.
IIRC the only actual difference if the 'end padding'.
So the wrapper need only ensure the copyin/out isn't too long.
Probably worth compile-time checks for the size.
If the structure is being copied to user then zero values need assigning
to the pad fields (unless read from user).

	David