* [PATCH 0/2] xfrm/crypto: unaligned access fixes
@ 2015-10-19 21:23 Sowmini Varadhan
2015-10-19 21:23 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() Sowmini Varadhan
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Sowmini Varadhan @ 2015-10-19 21:23 UTC (permalink / raw)
To: linux-crypto, linux-kernel, netdev
Cc: herbert, dhowells, davem, zohar, David.Woodhouse,
sowmini.varadhan, steffen.klassert
A two-part patchset that fixes some "unaligned access" warnings
that showed up my sparc test machines with ipsec set up.
Sowmini Varadhan (2):
crypto/x509: Fix unaligned access in x509_get_sig_params()
Fix unaligned access in xfrm_notify_sa() for DELSA
crypto/asymmetric_keys/x509_public_key.c | 5 +++--
net/xfrm/xfrm_user.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() 2015-10-19 21:23 [PATCH 0/2] xfrm/crypto: unaligned access fixes Sowmini Varadhan @ 2015-10-19 21:23 ` Sowmini Varadhan 2015-10-20 14:26 ` Herbert Xu 2015-10-19 21:23 ` [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA Sowmini Varadhan 2015-10-20 9:50 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() David Howells 2 siblings, 1 reply; 12+ messages in thread From: Sowmini Varadhan @ 2015-10-19 21:23 UTC (permalink / raw) To: linux-crypto, linux-kernel, netdev Cc: herbert, dhowells, davem, zohar, David.Woodhouse, sowmini.varadhan, steffen.klassert x509_get_sig_params() has the same code pattern as the one in pkcs7_verify() that is fixed by commit 62f57d05e287 ("crypto: pkcs7 - Fix unaligned access in pkcs7_verify()") so apply a similar fix here: make sure that desc is pointing at an algined value past the digest_size, and take alignment values into consideration when doing kzalloc() Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> --- crypto/asymmetric_keys/x509_public_key.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c index 1970966..68c3c40 100644 --- a/crypto/asymmetric_keys/x509_public_key.c +++ b/crypto/asymmetric_keys/x509_public_key.c @@ -194,14 +194,15 @@ int x509_get_sig_params(struct x509_certificate *cert) * digest storage space. */ ret = -ENOMEM; - digest = kzalloc(digest_size + desc_size, GFP_KERNEL); + digest = kzalloc(ALIGN(digest_size, __alignof__(*desc)) + desc_size, + GFP_KERNEL); if (!digest) goto error; cert->sig.digest = digest; cert->sig.digest_size = digest_size; - desc = digest + digest_size; + desc = PTR_ALIGN(digest + digest_size, __alignof__(*desc)); desc->tfm = tfm; desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() 2015-10-19 21:23 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() Sowmini Varadhan @ 2015-10-20 14:26 ` Herbert Xu 0 siblings, 0 replies; 12+ messages in thread From: Herbert Xu @ 2015-10-20 14:26 UTC (permalink / raw) To: Sowmini Varadhan Cc: linux-crypto, linux-kernel, netdev, dhowells, davem, zohar, David.Woodhouse, steffen.klassert On Mon, Oct 19, 2015 at 05:23:28PM -0400, Sowmini Varadhan wrote: > x509_get_sig_params() has the same code pattern as the one in > pkcs7_verify() that is fixed by commit 62f57d05e287 ("crypto: pkcs7 - Fix > unaligned access in pkcs7_verify()") so apply a similar fix here: make > sure that desc is pointing at an algined value past the digest_size, > and take alignment values into consideration when doing kzalloc() > > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> Patch applied. Thanks. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-19 21:23 [PATCH 0/2] xfrm/crypto: unaligned access fixes Sowmini Varadhan 2015-10-19 21:23 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() Sowmini Varadhan @ 2015-10-19 21:23 ` Sowmini Varadhan 2015-10-21 6:57 ` Steffen Klassert 2015-10-20 9:50 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() David Howells 2 siblings, 1 reply; 12+ messages in thread From: Sowmini Varadhan @ 2015-10-19 21:23 UTC (permalink / raw) To: linux-crypto, linux-kernel, netdev Cc: herbert, dhowells, davem, zohar, David.Woodhouse, sowmini.varadhan, steffen.klassert On sparc, deleting established SAs (e.g., by restarting ipsec at the peer) results in unaligned access messages via xfrm_del_sa -> km_state_notify -> xfrm_send_state_notify(). Use an aligned pointer to xfrm_usersa_info for this case. Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> --- net/xfrm/xfrm_user.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index a8de9e3..158ef4a 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2659,7 +2659,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) if (attr == NULL) goto out_free_skb; - p = nla_data(attr); + p = PTR_ALIGN(nla_data(attr), __alignof__(*p)); } err = copy_to_user_state_extra(x, p, skb); if (err) -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-19 21:23 ` [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA Sowmini Varadhan @ 2015-10-21 6:57 ` Steffen Klassert 2015-10-21 10:54 ` Sowmini Varadhan 2015-10-21 13:10 ` David Miller 0 siblings, 2 replies; 12+ messages in thread From: Steffen Klassert @ 2015-10-21 6:57 UTC (permalink / raw) To: Sowmini Varadhan Cc: linux-crypto, linux-kernel, netdev, herbert, dhowells, davem, zohar, David.Woodhouse On Mon, Oct 19, 2015 at 05:23:29PM -0400, Sowmini Varadhan wrote: > On sparc, deleting established SAs (e.g., by restarting ipsec > at the peer) results in unaligned access messages via > xfrm_del_sa -> km_state_notify -> xfrm_send_state_notify(). > Use an aligned pointer to xfrm_usersa_info for this case. > > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> > --- > net/xfrm/xfrm_user.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > index a8de9e3..158ef4a 100644 > --- a/net/xfrm/xfrm_user.c > +++ b/net/xfrm/xfrm_user.c > @@ -2659,7 +2659,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) > if (attr == NULL) > goto out_free_skb; > > - p = nla_data(attr); > + p = PTR_ALIGN(nla_data(attr), __alignof__(*p)); Hm, this breaks userspace notifications on 64-bit systems. Userspace expects this to be aligned to 4, with your patch it is aligned to 8 on 64-bit. Without your patch I get the correct notification when deleting a SA: ip x m Deleted src 172.16.0.2 dst 172.16.0.1 proto esp spi 0x00000002 reqid 2 mode tunnel replay-window 32 auth-trunc hmac(sha1) 0x31323334353637383930 96 enc cbc(aes) 0x31323334353637383930313233343536 sel src 10.0.0.0/24 dst 192.168.0.0/24 With your patch I get for the same SA: ip x m Deleted src 50.0.0.0 dst 0.0.0.0 proto 0 reqid 0 mode transport replay-window 0 flag decap-dscp auth-trunc hmac(sha1) 0x31323334353637383930 96 enc cbc(aes) 0x31323334353637383930313233343536 sel src 0.0.0.0/0 dst 0.234.255.255/0 proto igmp ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-21 6:57 ` Steffen Klassert @ 2015-10-21 10:54 ` Sowmini Varadhan 2015-10-21 12:36 ` Sowmini Varadhan 2015-10-21 13:17 ` David Miller 2015-10-21 13:10 ` David Miller 1 sibling, 2 replies; 12+ messages in thread From: Sowmini Varadhan @ 2015-10-21 10:54 UTC (permalink / raw) To: Steffen Klassert Cc: linux-crypto, linux-kernel, netdev, herbert, dhowells, davem, zohar, David.Woodhouse On (10/21/15 08:57), Steffen Klassert wrote: > > --- a/net/xfrm/xfrm_user.c > > +++ b/net/xfrm/xfrm_user.c > > @@ -2659,7 +2659,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) > > if (attr == NULL) > > goto out_free_skb; > > > > - p = nla_data(attr); > > + p = PTR_ALIGN(nla_data(attr), __alignof__(*p)); > > Hm, this breaks userspace notifications on 64-bit systems. > Userspace expects this to be aligned to 4, with your patch > it is aligned to 8 on 64-bit. > > Without your patch I get the correct notification when deleting a SA: > But __alignof__(*p) is 8 on sparc, and without the patch I get all types of unaligned access. So what do you suggest as the fix? (and openswan/pluto dont flag any errors with the patch, which is a separate bug). --Sowmini ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-21 10:54 ` Sowmini Varadhan @ 2015-10-21 12:36 ` Sowmini Varadhan 2015-10-21 13:22 ` David Miller 2015-10-21 13:17 ` David Miller 1 sibling, 1 reply; 12+ messages in thread From: Sowmini Varadhan @ 2015-10-21 12:36 UTC (permalink / raw) To: Steffen Klassert Cc: linux-crypto, linux-kernel, netdev, herbert, dhowells, davem, zohar, David.Woodhouse On (10/21/15 06:54), Sowmini Varadhan wrote: > But __alignof__(*p) is 8 on sparc, and without the patch I get > all types of unaligned access. So what do you suggest as the fix? Even though the alignment is, in fact, 8 (and that comes from struct xfrm_lifetime_cfg), if uspace is firmly attached to the 4 byte alignment, I think we can retain that behavior and still avoid unaligned access in the kernel with the following (admittedly ugly hack). Can you please take a look? I tested it with 'ip x m' and a transport mode tunnel on my sparc. diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 158ef4a..ca4e7f0 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2620,7 +2620,7 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x) static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) { struct net *net = xs_net(x); - struct xfrm_usersa_info *p; + struct xfrm_usersa_info *p, tmp; struct xfrm_usersa_id *id; struct nlmsghdr *nlh; struct sk_buff *skb; @@ -2659,11 +2659,16 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) if (attr == NULL) goto out_free_skb; - p = PTR_ALIGN(nla_data(attr), __alignof__(*p)); + p = nla_data(attr); + err = copy_to_user_state_extra(x, &tmp, skb); + if (err) + goto out_free_skb; + memcpy((u8 *)p, &tmp, sizeof(tmp)); + } else { + err = copy_to_user_state_extra(x, p, skb); + if (err) + goto out_free_skb; } - err = copy_to_user_state_extra(x, p, skb); - if (err) - goto out_free_skb; nlmsg_end(skb, nlh); ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-21 12:36 ` Sowmini Varadhan @ 2015-10-21 13:22 ` David Miller 2015-10-21 13:11 ` Sowmini Varadhan 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2015-10-21 13:22 UTC (permalink / raw) To: sowmini.varadhan Cc: steffen.klassert, linux-crypto, linux-kernel, netdev, herbert, dhowells, zohar, David.Woodhouse From: Sowmini Varadhan <sowmini.varadhan@oracle.com> Date: Wed, 21 Oct 2015 08:36:28 -0400 > + memcpy((u8 *)p, &tmp, sizeof(tmp)); memcpy() _never_ works for avoiding unaligned accessed. I repeat, no matter what you do, no matter what kinds of casts or fancy typing you use, memcpy() _never_ works for this purpose. The compiler knows that the pointer you are using is supposed to be at least 8 byte aligned, it can look through the cast and that's completely legitimate for it to do. So it can legitimately inline emit loads and stores to implement the memcpy() and those will still get the unaligned accesses. There is one and only one portable way to access unaligned data, and that is with the get_unaligned() and put_unaligned() helpers. Userland must do something similar to deal with this situation as well. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-21 13:22 ` David Miller @ 2015-10-21 13:11 ` Sowmini Varadhan 0 siblings, 0 replies; 12+ messages in thread From: Sowmini Varadhan @ 2015-10-21 13:11 UTC (permalink / raw) To: David Miller Cc: steffen.klassert, linux-crypto, linux-kernel, netdev, herbert, dhowells, zohar, David.Woodhouse On (10/21/15 06:22), David Miller wrote: > memcpy() _never_ works for avoiding unaligned accessed. > > I repeat, no matter what you do, no matter what kinds of casts or > fancy typing you use, memcpy() _never_ works for this purpose. : > There is one and only one portable way to access unaligned data, > and that is with the get_unaligned() and put_unaligned() helpers. ok. I'll fix it up to use the *_unaligned functions and resend this out later today. --Sowmini ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-21 10:54 ` Sowmini Varadhan 2015-10-21 12:36 ` Sowmini Varadhan @ 2015-10-21 13:17 ` David Miller 1 sibling, 0 replies; 12+ messages in thread From: David Miller @ 2015-10-21 13:17 UTC (permalink / raw) To: sowmini.varadhan Cc: steffen.klassert, linux-crypto, linux-kernel, netdev, herbert, dhowells, zohar, David.Woodhouse From: Sowmini Varadhan <sowmini.varadhan@oracle.com> Date: Wed, 21 Oct 2015 06:54:42 -0400 > On (10/21/15 08:57), Steffen Klassert wrote: >> > --- a/net/xfrm/xfrm_user.c >> > +++ b/net/xfrm/xfrm_user.c >> > @@ -2659,7 +2659,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) >> > if (attr == NULL) >> > goto out_free_skb; >> > >> > - p = nla_data(attr); >> > + p = PTR_ALIGN(nla_data(attr), __alignof__(*p)); >> >> Hm, this breaks userspace notifications on 64-bit systems. >> Userspace expects this to be aligned to 4, with your patch >> it is aligned to 8 on 64-bit. >> >> Without your patch I get the correct notification when deleting a SA: >> > > But __alignof__(*p) is 8 on sparc, and without the patch I get > all types of unaligned access. So what do you suggest as the fix? The accesses have to be done using something like get_unaligned() and put_unaligned(). Sorry, but the protocol is set in stone and this is unfortunately how it is. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA 2015-10-21 6:57 ` Steffen Klassert 2015-10-21 10:54 ` Sowmini Varadhan @ 2015-10-21 13:10 ` David Miller 1 sibling, 0 replies; 12+ messages in thread From: David Miller @ 2015-10-21 13:10 UTC (permalink / raw) To: steffen.klassert Cc: sowmini.varadhan, linux-crypto, linux-kernel, netdev, herbert, dhowells, zohar, David.Woodhouse From: Steffen Klassert <steffen.klassert@secunet.com> Date: Wed, 21 Oct 2015 08:57:04 +0200 > On Mon, Oct 19, 2015 at 05:23:29PM -0400, Sowmini Varadhan wrote: >> On sparc, deleting established SAs (e.g., by restarting ipsec >> at the peer) results in unaligned access messages via >> xfrm_del_sa -> km_state_notify -> xfrm_send_state_notify(). >> Use an aligned pointer to xfrm_usersa_info for this case. >> >> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> >> --- >> net/xfrm/xfrm_user.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c >> index a8de9e3..158ef4a 100644 >> --- a/net/xfrm/xfrm_user.c >> +++ b/net/xfrm/xfrm_user.c >> @@ -2659,7 +2659,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) >> if (attr == NULL) >> goto out_free_skb; >> >> - p = nla_data(attr); >> + p = PTR_ALIGN(nla_data(attr), __alignof__(*p)); > > Hm, this breaks userspace notifications on 64-bit systems. > Userspace expects this to be aligned to 4, with your patch > it is aligned to 8 on 64-bit. That's correct, netlink attributes are fundamentally only 4 byte aligned and this cannot be changed. nla_data() is exactly where the attribute must be placed, aligned or not. The only workaround is, when designing netlink attributes. Various netlink libraries have workarounds for accessing, for example, 64-bit stats which are going to be unaligned in netlink messages. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() 2015-10-19 21:23 [PATCH 0/2] xfrm/crypto: unaligned access fixes Sowmini Varadhan 2015-10-19 21:23 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() Sowmini Varadhan 2015-10-19 21:23 ` [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA Sowmini Varadhan @ 2015-10-20 9:50 ` David Howells 2 siblings, 0 replies; 12+ messages in thread From: David Howells @ 2015-10-20 9:50 UTC (permalink / raw) To: Sowmini Varadhan Cc: dhowells, linux-crypto, linux-kernel, netdev, herbert, davem, zohar, David.Woodhouse, steffen.klassert Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > x509_get_sig_params() has the same code pattern as the one in > pkcs7_verify() that is fixed by commit 62f57d05e287 ("crypto: pkcs7 - Fix > unaligned access in pkcs7_verify()") so apply a similar fix here: make > sure that desc is pointing at an algined value past the digest_size, > and take alignment values into consideration when doing kzalloc() > > Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> Acked-by: David Howells <dhowells@redhat.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-10-21 13:11 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-19 21:23 [PATCH 0/2] xfrm/crypto: unaligned access fixes Sowmini Varadhan 2015-10-19 21:23 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() Sowmini Varadhan 2015-10-20 14:26 ` Herbert Xu 2015-10-19 21:23 ` [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA Sowmini Varadhan 2015-10-21 6:57 ` Steffen Klassert 2015-10-21 10:54 ` Sowmini Varadhan 2015-10-21 12:36 ` Sowmini Varadhan 2015-10-21 13:22 ` David Miller 2015-10-21 13:11 ` Sowmini Varadhan 2015-10-21 13:17 ` David Miller 2015-10-21 13:10 ` David Miller 2015-10-20 9:50 ` [PATCH 1/2] crypto/x509: Fix unaligned access in x509_get_sig_params() David Howells
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.