Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
by Ard Biesheuvel
On Thu, 27 Aug 2020 at 00:19, Herbert Xu <herbert(a)gondor.apana.org.au> wrote:
>
> On Wed, Aug 26, 2020 at 05:42:27PM +0200, Ard Biesheuvel wrote:
> >
> > I still get a failure in aes_siv_encrypt(), which does not occur with
> > the kernel side fix applied.
>
> Where is this test from? I can't find it in the ell git tree.
>
It is part of iwd - just build that and run 'make check'
With your patch applied, the occurrence of sendmsg() in
operate_cipher() triggers the warn_once(), but if I add MSG_MORE
there, the test hangs.
1 year, 10 months
Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
by Caleb Jorden
I can tell you all assumed this, but just by way as a quick update on the original issue:
I have confirmed that Herbert's patch (crypto: af_alg - Work around empty control messages without MSG_MORE) does indeed fix the original iwd 1.8 + WPA Enterprise issue.
Thank you!
Caleb Jorden
________________________________________
From: Herbert Xu <herbert(a)gondor.apana.org.au>
Sent: Thursday, August 27, 2020 3:49 AM
To: Ard Biesheuvel
Cc: Denis Kenzior; Andrew Zaborowski; Paul Menzel; Caleb Jorden; Sasha Levin; iwd(a)lists.01.org; # 3.4.x; Greg KH; LKML; David S. Miller; Linux Crypto Mailing List
Subject: Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
On Wed, Aug 26, 2020 at 05:42:27PM +0200, Ard Biesheuvel wrote:
>
> I still get a failure in aes_siv_encrypt(), which does not occur with
> the kernel side fix applied.
Where is this test from? I can't find it in the ell git tree.
Thanks,
--
Email: Herbert Xu <herbert(a)gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
1 year, 10 months
Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
by Denis Kenzior
Hi Herbert,
On 8/26/20 9:19 AM, Herbert Xu wrote:
> On Wed, Aug 26, 2020 at 08:57:17AM -0500, Denis Kenzior wrote:
>>
>> I'm just waking up now, so I might seem dense, but for my education, can you
>> tell me why we need to set MSG_MORE when we issue just a single sendmsg
>> followed immediately by recv/recvmsg? ell/iwd operates on small buffers, so
>> we don't really feed the kernel data in multiple send operations. You can
>> see this in the ell git tree link referenced in Andrew's reply.
>
> You obviously don't need MSG_MORE if you're doing a single sendmsg.
>
> The problematic code is in l_cipher_set_iv. It does a sendmsg(2)
> that expects to be followed by more sendmsg(2) calls before a
> recvmsg(2). That's the one that needs a MSG_MORE.
>
Gotcha. I fixed the set_iv part now in ell:
https://git.kernel.org/pub/scm/libs/ell/ell.git/commit/?id=87c76bbc85fe28...
Regards,
-Denis
1 year, 10 months
Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
by Denis Kenzior
Hi Herbert,
On 8/26/20 8:00 AM, Herbert Xu wrote:
> On Wed, Aug 26, 2020 at 02:58:02PM +0200, Andrew Zaborowski wrote:
>>
>> Running iwd's and ell's unit tests I can see that at least the
>> following algorithms give EINVAL errors:
>> ecb(aes)
>> cbc(aes)
>> ctr(aes)
>>
>> The first one fails in recv() and only for some input lengths. The
>> latter two fail in send(). The relevant ell code starts at
>> https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/cipher.c#n271
>>
>> The tests didn't get to the point where aead is used.
>
> Yes ell needs to set MSG_MORE after sending the control message.
> Any sendmsg(2) without a MSG_MORE will be interpreted as the end
> of a request.
I'm just waking up now, so I might seem dense, but for my education, can you
tell me why we need to set MSG_MORE when we issue just a single sendmsg followed
immediately by recv/recvmsg? ell/iwd operates on small buffers, so we don't
really feed the kernel data in multiple send operations. You can see this in
the ell git tree link referenced in Andrew's reply.
According to https://www.kernel.org/doc/html/latest/crypto/userspace-if.html:
The send system call family allows the following flag to be specified:
MSG_MORE: If this flag is set, the send system call acts like a cipher
update function where more input data is expected with a subsequent invocation
of the send system call.
So given what I said above, the documentation seems to indicate that MSG_MORE
flag should not be used in our case?
Regards,
-Denis
>
> I'll work around this in the kernel though for the case where there
> is no actual data, with a WARN_ON_ONCE.
>
> Thanks,
>
1 year, 10 months
Re: [PATCH] crypto: af_alg - Work around empty control messages
without MSG_MORE
by Ard Biesheuvel
On Wed, 26 Aug 2020 at 15:30, Herbert Xu <herbert(a)gondor.apana.org.au> wrote:
>
> The iwd daemon uses libell which sets up the skcipher operation with
> two separate control messages. This is fine by itself but the first
> control message is sent without MSG_MORE. This means that the first
> control message is interpreted as an empty request.
>
> While libell should be fixed to use MSG_MORE where appropriate, this
> patch works around the bug in the kernel so that existing binaries
> continue to work.
>
> We will print a warning however.
>
> Reported-by: Caleb Jorden <caljorden(a)hotmail.com>
> Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when...")
> Cc: <stable(a)vger.kernel.org>
> Signed-off-by: Herbert Xu <herbert(a)gondor.apana.org.au>
>
Applied this onto v5.4.60, and it makes the iwd selftests pass again
Acked-by: Ard Biesheuvel <ardb(a)kernel.org>
Tested-by: Ard Biesheuvel <ardb(a)kernel.org>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index a6f581ab200c..3da21cadc326 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -16,6 +16,7 @@
> #include <linux/module.h>
> #include <linux/net.h>
> #include <linux/rwsem.h>
> +#include <linux/sched.h>
> #include <linux/sched/signal.h>
> #include <linux/security.h>
>
> @@ -846,8 +847,14 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
>
> lock_sock(sk);
> if (ctx->init && (init || !ctx->more)) {
> - err = -EINVAL;
> - goto unlock;
> + if (ctx->used) {
> + err = -EINVAL;
> + goto unlock;
> + }
> +
> + pr_info_once(
> + "%s sent an empty control message without MSG_MORE.\n",
> + current->comm);
> }
> ctx->init = true;
>
> --
> Email: Herbert Xu <herbert(a)gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
1 year, 10 months
Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
by Andrew Zaborowski
On Wed, 26 Aug 2020 at 14:10, Herbert Xu <herbert(a)gondor.apana.org.au> wrote:
> On Wed, Aug 26, 2020 at 01:59:53PM +0200, Ard Biesheuvel wrote:
> > On Wed, 26 Aug 2020 at 13:50, Herbert Xu <herbert(a)gondor.apana.org.au> wrote:
> > >
> > > On Wed, Aug 26, 2020 at 12:40:14PM +0200, Ard Biesheuvel wrote:
> > > >
> > > > It would be helpful if someone could explain for the non-mac80211
> > > > enlightened readers how iwd's EAP-PEAPv0 + MSCHAPv2 support relies on
> > > > the algif_aead socket interface, and which AEAD algorithms it uses. I
> > > > assume this is part of libell?
> > >
> > > I see the problem. libell/ell/checksum.c doesn't clear the MSG_MORE
> > > flag before doing the recv(2).
> >
> > But that code uses a hash not an aead, afaict.
>
> Good point. In that case can we please get a strace with a -s
> option that's big enough to capture the crypto data?
Running iwd's and ell's unit tests I can see that at least the
following algorithms give EINVAL errors:
ecb(aes)
cbc(aes)
ctr(aes)
The first one fails in recv() and only for some input lengths. The
latter two fail in send(). The relevant ell code starts at
https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/cipher.c#n271
The tests didn't get to the point where aead is used.
Best regards
1 year, 10 months
Re: Issue with iwd + Linux 5.8.3 + WPA Enterprise
by Ard Biesheuvel
On Wed, 26 Aug 2020 at 13:50, Herbert Xu <herbert(a)gondor.apana.org.au> wrote:
>
> On Wed, Aug 26, 2020 at 12:40:14PM +0200, Ard Biesheuvel wrote:
> >
> > It would be helpful if someone could explain for the non-mac80211
> > enlightened readers how iwd's EAP-PEAPv0 + MSCHAPv2 support relies on
> > the algif_aead socket interface, and which AEAD algorithms it uses. I
> > assume this is part of libell?
>
> I see the problem. libell/ell/checksum.c doesn't clear the MSG_MORE
> flag before doing the recv(2).
>
But that code uses a hash not an aead, afaict.
> I was hoping nobody out there was doing this but obviously I've
> been proven wrong.
>
> So what I'm going to do is to specifically allow this case of
> a string of sendmsg(2)'s with MSG_MORE folloed by a recvmsg(2)
> in the same thread. I'll add a WARN_ON_ONCE so user-space can
> eventually be fixed.
>
> Cheers,
> --
> Email: Herbert Xu <herbert(a)gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
1 year, 10 months
Issue with iwd + Linux 5.8.3 + WPA Enterprise
by caljorden＠hotmail.com
Hello,
I wanted to note an issue that I have hit with iwd when I upgraded to the Linux 5.8.3 stable kernel. My office network uses WPA Enterprise with EAP-PEAPv0 + MSCHAPv2. When using this office network, upgrading to Linux 5.8.3 caused my system to refuse to associate successfully to the network. I get the following in my dmesg logs:
[ 40.846535] wlan0: authenticate with <redacted>:60
[ 40.850570] wlan0: send auth to <redacted>:60 (try 1/3)
[ 40.854627] wlan0: authenticated
[ 40.855992] wlan0: associate with <redacted>:60 (try 1/3)
[ 40.860450] wlan0: RX AssocResp from <redacted>:60 (capab=0x411 status=0 aid=11)
[ 40.861620] wlan0: associated
[ 41.886503] wlan0: deauthenticating from <redacted>:60 by local choice (Reason: 23=IEEE8021X_FAILED)
[ 42.360127] wlan0: authenticate with <redacted>:22
[ 42.364584] wlan0: send auth to <redacted>:22 (try 1/3)
[ 42.370821] wlan0: authenticated
[ 42.372658] wlan0: associate with <redacted>:22 (try 1/3)
[ 42.377426] wlan0: RX AssocResp from <redacted>:22 (capab=0x411 status=0 aid=15)
[ 42.378607] wlan0: associated
[ 43.402009] wlan0: deauthenticating from <redacted>:22 by local choice (Reason: 23=IEEE8021X_FAILED)
[ 43.875921] wlan0: authenticate with <redacted>:60
[ 43.879988] wlan0: send auth to <redacted>:60 (try 1/3)
[ 43.886244] wlan0: authenticated
[ 43.889273] wlan0: associate with <redacted>:60 (try 1/3)
[ 43.894586] wlan0: RX AssocResp from <redacted>:60 (capab=0x411 status=0 aid=11)
[ 43.896077] wlan0: associated
[ 44.918504] wlan0: deauthenticating from <redacted>:60 by local choice (Reason: 23=IEEE8021X_FAILED)
This continues as long as I let iwd run.
I downgraded back to Linux 5.8.2, and verified that everything works as expected. I also tried using Linux 5.8.3 on a different system at my home, which uses WPA2-PSK. It worked fine (though it uses an Atheros wireless card instead of an Intel card - but I assume that is irrelevant).
I decided to try to figure out what caused the issue in the changes for Linux 5.8.3. I assumed that it was something that changed in the crypto interface, which limited my bisection to a very few commits. Sure enough, I found that if I revert commit e91d82703ad0bc68942a7d91c1c3d993e3ad87f0 (crypto: algif_aead - Only wake up when ctx->more is zero), the problem goes away and I am able to associate to my WPA Enterprise network successfully, and use it. I found that in order to revert this commit, I also first had to revert 465c03e999102bddac9b1e132266c232c5456440 (crypto: af_alg - Fix regression on empty requests), because the two commits have coupled changes.
I normally would have assumed that this should be sent to the kernel list, but I thought I would first mention it here because of what I found in some email threads on the Linux-Crypto list about the crypto interfaces to the kernel being sub-optimal and needing to be fixed. The changes in these commits look like they are just trying to fix what could be broken interfaces, so I thought that it would make sense to see what the iwd team thinks about the situation first.
The wireless card I was using during this testing is an Intel Wireless 3165 (rev 81). If there is any additional information I could help provide, please let me know.
Thanks!
Caleb Jorden
1 year, 10 months
[PATCH 1/3] p2p: Send the right UUID-E in probe request WSC IEs
by Andrew Zaborowski
When building the scan IEs for our provisioning scans, use the UUID-E
based on the Interface Address, not the Device Address, as that is what
wsc.c will be using to in the registration protocol.
Eventually we may have to base the UUID-E on the Device Address or
something else that is persistent, and pass the actual UUID-E to wsc.c,
as the Interface Address is randomly generated on every connect attempt.
IIRC the UUID-E is supposed to be persistent.
---
src/p2p.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/p2p.c b/src/p2p.c
index 3b38f738..7772080b 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -329,6 +329,7 @@ static uint8_t *p2p_build_scan_ies(struct p2p_device *dev, uint8_t *buf,
size_t wsc_ie_size;
uint8_t wfd_ie[32];
size_t wfd_ie_size;
+ const uint8_t *addr;
p2p_info.capability = dev->capability;
memcpy(p2p_info.listen_channel.country, dev->listen_country, 3);
@@ -350,7 +351,14 @@ static uint8_t *p2p_build_scan_ies(struct p2p_device *dev, uint8_t *buf,
wsc_info.request_type = WSC_REQUEST_TYPE_ENROLLEE_INFO;
wsc_info.config_methods = dev->device_info.wsc_config_methods;
- if (!wsc_uuid_from_addr(dev->addr, wsc_info.uuid_e))
+ /*
+ * If we're doing the provisioning scan, we need to use the same UUID-E
+ * that we'll use in the WSC enrollee registration protocol because the
+ * GO might validate it.
+ */
+ addr = dev->conn_peer ? dev->conn_addr : dev->addr;
+
+ if (!wsc_uuid_from_addr(addr, wsc_info.uuid_e))
return NULL;
wsc_info.primary_device_type = dev->device_info.primary_device_type;
--
2.25.1
1 year, 10 months
[PATCH 1/5] wscutil: Allow 0-length attributes in wsc_attr_builder
by Andrew Zaborowski
wsc_attr_builder_start_attr and wsc_attr_builder_free look at
builder->curlen to see whether the TLV's length needs to be updated to
include the previous attribute. If builder->curlen is 0
wsc_attr_builder_start_attr assumes there's no previous attribute and
starts writing at current builder->offset. If the previous attribute
length was 0 curlen would stay at 0 and that attribute would get
overwritten with the new one. To solve this add the 4 bytes of the T
and L to curlen as soon as a new attribute is started, and subtract
them when writing the L value. The alternative would be to set a flag
to say whether an attribute was started.
The spec explicitly allows 0-length attributes in section 12:
"The variable length string attributes, e.g., Device Name, are encoded
without null-termination, i.e., no 0x00 octets added to the end of the
value. If the string is empty, the attribute length is set to zero."
---
src/wscutil.c | 38 ++++++++++++++++++--------------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/src/wscutil.c b/src/wscutil.c
index a41702e8..d78a0354 100644
--- a/src/wscutil.c
+++ b/src/wscutil.c
@@ -1634,11 +1634,12 @@ static bool wsc_attr_builder_start_attr(struct wsc_attr_builder *builder,
/* Record previous attribute's length */
if (builder->curlen > 0) {
bytes = builder->buf + builder->offset;
- l_put_be16(builder->curlen, bytes + 2);
- builder->offset += 4 + builder->curlen;
- builder->curlen = 0;
+ l_put_be16(builder->curlen - 4, bytes + 2);
+ builder->offset += builder->curlen;
}
+ builder->curlen = 4;
+
if (builder->offset + 4 >= builder->capacity)
wsc_attr_builder_grow(builder);
@@ -1650,10 +1651,10 @@ static bool wsc_attr_builder_start_attr(struct wsc_attr_builder *builder,
static bool wsc_attr_builder_put_u8(struct wsc_attr_builder *builder, uint8_t v)
{
- if (builder->offset + 4 + builder->curlen + 1 >= builder->capacity)
+ if (builder->offset + builder->curlen + 1 >= builder->capacity)
wsc_attr_builder_grow(builder);
- builder->buf[builder->offset + 4 + builder->curlen] = v;
+ builder->buf[builder->offset + builder->curlen] = v;
builder->curlen += 1;
return true;
@@ -1662,10 +1663,10 @@ static bool wsc_attr_builder_put_u8(struct wsc_attr_builder *builder, uint8_t v)
static bool wsc_attr_builder_put_u16(struct wsc_attr_builder *builder,
uint16_t v)
{
- if (builder->offset + 4 + builder->curlen + 2 >= builder->capacity)
+ if (builder->offset + builder->curlen + 2 >= builder->capacity)
wsc_attr_builder_grow(builder);
- l_put_be16(v, builder->buf + builder->offset + 4 + builder->curlen);
+ l_put_be16(v, builder->buf + builder->offset + builder->curlen);
builder->curlen += 2;
return true;
@@ -1674,10 +1675,10 @@ static bool wsc_attr_builder_put_u16(struct wsc_attr_builder *builder,
static bool wsc_attr_builder_put_u32(struct wsc_attr_builder *builder,
uint32_t v)
{
- if (builder->offset + 4 + builder->curlen + 4 >= builder->capacity)
+ if (builder->offset + builder->curlen + 4 >= builder->capacity)
wsc_attr_builder_grow(builder);
- l_put_be32(v, builder->buf + builder->offset + 4 + builder->curlen);
+ l_put_be32(v, builder->buf + builder->offset + builder->curlen);
builder->curlen += 4;
return true;
@@ -1686,12 +1687,10 @@ static bool wsc_attr_builder_put_u32(struct wsc_attr_builder *builder,
static bool wsc_attr_builder_put_bytes(struct wsc_attr_builder *builder,
const void *bytes, size_t size)
{
- while (builder->offset + 4 + builder->curlen + size >=
- builder->capacity)
+ while (builder->offset + builder->curlen + size >= builder->capacity)
wsc_attr_builder_grow(builder);
- memcpy(builder->buf + builder->offset + 4 + builder->curlen,
- bytes, size);
+ memcpy(builder->buf + builder->offset + builder->curlen, bytes, size);
builder->curlen += size;
return true;
@@ -1700,10 +1699,10 @@ static bool wsc_attr_builder_put_bytes(struct wsc_attr_builder *builder,
static bool wsc_attr_builder_put_oui(struct wsc_attr_builder *builder,
const uint8_t *oui)
{
- if (builder->offset + 4 + builder->curlen + 3 >= builder->capacity)
+ if (builder->offset + builder->curlen + 3 >= builder->capacity)
wsc_attr_builder_grow(builder);
- memcpy(builder->buf + builder->offset + 4 + builder->curlen, oui, 3);
+ memcpy(builder->buf + builder->offset + builder->curlen, oui, 3);
builder->curlen += 3;
return true;
@@ -1721,11 +1720,10 @@ static bool wsc_attr_builder_put_string(struct wsc_attr_builder *builder,
len = 1;
}
- if (builder->offset + 4 + builder->curlen + len >= builder->capacity)
+ if (builder->offset + builder->curlen + len >= builder->capacity)
wsc_attr_builder_grow(builder);
- memcpy(builder->buf + builder->offset + 4 + builder->curlen,
- string, len);
+ memcpy(builder->buf + builder->offset + builder->curlen, string, len);
builder->curlen += len;
return true;
@@ -1753,8 +1751,8 @@ static uint8_t *wsc_attr_builder_free(struct wsc_attr_builder *builder,
if (builder->curlen > 0) {
uint8_t *bytes = builder->buf + builder->offset;
- l_put_be16(builder->curlen, bytes + 2);
- builder->offset += 4 + builder->curlen;
+ l_put_be16(builder->curlen - 4, bytes + 2);
+ builder->offset += builder->curlen;
builder->curlen = 0;
}
--
2.25.1
1 year, 10 months