almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()

RFC PATCH In review Al Viro 3 participants 4 messages Feb 15, 2020 – Feb 22, 2020 1 message not in the archive

Subsystems (2)

Crypto API CXGB4 crypto driver (chcr)

lore

Share & tools

lore

Al Viro

author

2020-02-15 06:14 UTC #0 lore raw

RFC PATCH 1/1 almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()

kctx_len = (ntohl(KEY_CONTEXT_CTX_LEN_V(aeadctx->key_ctx_hdr)) << 4)
- sizeof(chcr_req->key_ctx);
can't possibly be endian-safe. Look: ->key_ctx_hdr is __be32. And KEY_CONTEXT_CTX_LEN_V is "shift up by 24 bits". On little-endian hosts it sees

	b0 b1 b2 b3

in memory, inteprets that into b0 + (b1 << 8) + (b2 << 16) + (b3 << 24), shifts up by 24, resulting in b0 << 24, does ntohl (byteswap on l-e), gets b0 and shifts that up by 4. So we get b0 * 16 - sizeof(...).

Sounds reasonable, but on b-e we get
b3 + (b2 << 8) + (b1 << 16) + (b0 << 24), shift up by 24, yielding b3 << 24, do ntohl (no-op on b-e) and then shift up by 4. Resulting in b3 << 28 - sizeof(...), i.e. slightly under b3 * 256M.

Then we increase it some more and pass to alloc_skb() as size. Somehow I doubt that we really want a quarter-gigabyte skb allocation here...

Note that when you are building those values in

#define  FILL_KEY_CTX_HDR(ck_size, mk_size, d_ck, opad, ctx_len) \
                htonl(KEY_CONTEXT_VALID_V(1) | \
                      KEY_CONTEXT_CK_SIZE_V((ck_size)) | \
                      KEY_CONTEXT_MK_SIZE_V(mk_size) | \
                      KEY_CONTEXT_DUAL_CK_V((d_ck)) | \
                      KEY_CONTEXT_OPAD_PRESENT_V((opad)) | \
                      KEY_CONTEXT_SALT_PRESENT_V(1) | \
                      KEY_CONTEXT_CTX_LEN_V((ctx_len)))

ctx_len ends up in the first octet (i.e. b0 in the above), which matches the current behaviour on l-e. If that's the intent, this thing should've been
kctx_len = (KEY_CONTEXT_CTX_LEN_G(ntohl(aeadctx->key_ctx_hdr)) << 4)
- sizeof(chcr_req->key_ctx);
instead - fetch after ntohl() we get (b0 << 24) + (b1 << 16) + (b2 << 8) + b3, shift it down by 24 (b0), resuling in b0 * 16 - sizeof(...) both on l-e and on b-e.

PS: when sparse warns you about endianness problems, it might be worth checking if there really is something wrong. And I don't mean "slap __force cast on it"...

Signed-off-by Al Viro <[email protected]>

---

drivers/crypto/chelsio/chcr_algo.c +1 -1

diff -urN a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
@@ -2351,7 +2351,7 @@ static struct sk_buff *create_authenc_wr(struct aead_request *req,
 	snents = sg_nents_xlen(req->src, req->assoclen + req->cryptlen,
 			       CHCR_SRC_SG_SIZE, 0);
 	dst_size = get_space_for_phys_dsgl(dnents);
-	kctx_len = (ntohl(KEY_CONTEXT_CTX_LEN_V(aeadctx->key_ctx_hdr)) << 4)
+	kctx_len = (KEY_CONTEXT_CTX_LEN_G(ntohl(aeadctx->key_ctx_hdr)) << 4)
 		- sizeof(chcr_req->key_ctx);
 	transhdr_len = CIPHER_TRANSHDR_SIZE(kctx_len, dst_size);
 	reqctx->imm = (transhdr_len + req->assoclen + req->cryptlen) <

Al Viro

author

2020-02-15 06:29 UTC #1 lore raw

Another fairly bug in there (this time in
drivers/crypto/chelsio/chtls/chtls_io.c):

/* Read TLS header to find content type and data length */ static int tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)

{
        if (copy_from_iter(thdr, sizeof(*thdr), from) != sizeof(*thdr))
                return -EFAULT;
        return (__force int)cpu_to_be16(thdr->length);
}

For one thing, that kind of force-casts is pretty much always wrong. This one clearly says "should've used be16_to_cpu()". And that includes annotating tls_hdr ->length as __be16; no idea about the other field in there (->version, that is).

For another, the only caller is

                        recordsz = tls_header_read(&hdr, &msg->msg_iter);
                        size -= TLS_HEADER_LENGTH;
                        copied += TLS_HEADER_LENGTH;
                        csk->tlshws.txleft = recordsz;
                        csk->tlshws.type = hdr.type;
                        if (skb)
                                ULP_SKB_CB(skb)->ulp.tls.type = hdr.type;
which doesn't look like something that'll work if you get -EFAULT out of
that function.  If anything, that smells like we want
                        recordsz = tls_header_read(&hdr, &msg->msg_iter);
			if (recordsz < 0)
				goto do_fault;
			...

What's more, we do *not* want a header that has faulted halfway through to be consumed. IOW, we want copy_from_iter_full():

static int tls_header_read(struct tls_hdr *thdr, struct iov_iter *from)

{
        if (!copy_from_iter_full(thdr, sizeof(*thdr), from))
                return -EFAULT;
        return be16_to_cpu(thdr->length);
}

in addition to that missing check in the caller...

A message that is not in the archive (sent off-list or not yet received): CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com

Ayush Sawal

maintainer

2020-02-21 05:17 UTC #3 lore raw

Quoted 55 lines · Al Viro

kctx_len = (ntohl(KEY_CONTEXT_CTX_LEN_V(aeadctx->key_ctx_hdr)) << 4)
- sizeof(chcr_req->key_ctx);
can't possibly be endian-safe. Look: ->key_ctx_hdr is __be32. And KEY_CONTEXT_CTX_LEN_V is "shift up by 24 bits". On little-endian hosts it sees
b0 b1 b2 b3
in memory, inteprets that into b0 + (b1 << 8) + (b2 << 16) + (b3 << 24), shifts up by 24, resulting in b0 << 24, does ntohl (byteswap on l-e), gets b0 and shifts that up by 4. So we get b0 * 16 - sizeof(...).
Sounds reasonable, but on b-e we get
b3 + (b2 << 8) + (b1 << 16) + (b0 << 24), shift up by 24, yielding b3 << 24, do ntohl (no-op on b-e) and then shift up by 4. Resulting in b3 << 28 - sizeof(...), i.e. slightly under b3 * 256M.
Then we increase it some more and pass to alloc_skb() as size. Somehow I doubt that we really want a quarter-gigabyte skb allocation here...
Note that when you are building those values in
#define FILL_KEY_CTX_HDR(ck_size, mk_size, d_ck, opad, ctx_len) \
htonl(KEY_CONTEXT_VALID_V(1) | \
KEY_CONTEXT_CK_SIZE_V((ck_size)) | \
KEY_CONTEXT_MK_SIZE_V(mk_size) | \
KEY_CONTEXT_DUAL_CK_V((d_ck)) | \
KEY_CONTEXT_OPAD_PRESENT_V((opad)) | \
KEY_CONTEXT_SALT_PRESENT_V(1) | \
KEY_CONTEXT_CTX_LEN_V((ctx_len)))
ctx_len ends up in the first octet (i.e. b0 in the above), which matches the current behaviour on l-e. If that's the intent, this thing should've been
kctx_len = (KEY_CONTEXT_CTX_LEN_G(ntohl(aeadctx->key_ctx_hdr)) << 4)
- sizeof(chcr_req->key_ctx);
instead - fetch after ntohl() we get (b0 << 24) + (b1 << 16) + (b2 << 8) + b3, shift it down by 24 (b0), resuling in b0 * 16 - sizeof(...) both on l-e and on b-e.
PS: when sparse warns you about endianness problems, it might be worth checking if there really is something wrong. And I don't mean "slap __force cast on it"...
Signed-off-by Al Viro <[email protected] <mailto:[email protected]>>
---
diff -urN a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
drivers/crypto/chelsio/chcr_algo.c +1 -1
@@ -2351,7 +2351,7 @@ static struct sk_buff *create_authenc_wr(struct 
 aead_request *req,
         snents = sg_nents_xlen(req->src, req->assoclen + req->cryptlen,
                                CHCR_SRC_SG_SIZE, 0);
         dst_size = get_space_for_phys_dsgl(dnents);
-       kctx_len = (ntohl(KEY_CONTEXT_CTX_LEN_V(aeadctx->key_ctx_hdr)) 
 << 4)
+       kctx_len = (KEY_CONTEXT_CTX_LEN_G(ntohl(aeadctx->key_ctx_hdr)) 
 << 4)

Quoted 3 lines

- sizeof(chcr_req->key_ctx);
transhdr_len = CIPHER_TRANSHDR_SIZE(kctx_len, dst_size); reqctx->imm = (transhdr_len + req->assoclen + req->cryptlen) <

This was a genuine bug, thanks a lot for pointing it out and providing the fix.We are checking other sparse warns in our files, and soon we will fix the warnings.

Thanks,
Ayush

Herbert Xu

maintainer

2020-02-22 01:44 UTC #4 lore raw

Quoted 44 lines · Ayush Sawal

Quoted 42 lines · Al Viro
kctx_len = (ntohl(KEY_CONTEXT_CTX_LEN_V(aeadctx->key_ctx_hdr)) << 4)
- sizeof(chcr_req->key_ctx);
can't possibly be endian-safe. Look: ->key_ctx_hdr is __be32. And KEY_CONTEXT_CTX_LEN_V is "shift up by 24 bits". On little-endian hosts it sees
b0 b1 b2 b3
in memory, inteprets that into b0 + (b1 << 8) + (b2 << 16) + (b3 << 24), shifts up by 24, resulting in b0 << 24, does ntohl (byteswap on l-e), gets b0 and shifts that up by 4. So we get b0 * 16 - sizeof(...).
Sounds reasonable, but on b-e we get
b3 + (b2 << 8) + (b1 << 16) + (b0 << 24), shift up by 24, yielding b3 << 24, do ntohl (no-op on b-e) and then shift up by 4. Resulting in b3 << 28 - sizeof(...), i.e. slightly under b3 * 256M.
Then we increase it some more and pass to alloc_skb() as size. Somehow I doubt that we really want a quarter-gigabyte skb allocation here...
Note that when you are building those values in
#define FILL_KEY_CTX_HDR(ck_size, mk_size, d_ck, opad, ctx_len) \
htonl(KEY_CONTEXT_VALID_V(1) | \
KEY_CONTEXT_CK_SIZE_V((ck_size)) | \
KEY_CONTEXT_MK_SIZE_V(mk_size) | \
KEY_CONTEXT_DUAL_CK_V((d_ck)) | \
KEY_CONTEXT_OPAD_PRESENT_V((opad)) | \
KEY_CONTEXT_SALT_PRESENT_V(1) | \
KEY_CONTEXT_CTX_LEN_V((ctx_len))) ctx_len ends up in the first octet (i.e. b0 in the above), which matches the current behaviour on l-e. If that's the intent, this thing should've been
kctx_len = (KEY_CONTEXT_CTX_LEN_G(ntohl(aeadctx->key_ctx_hdr)) << 4)
- sizeof(chcr_req->key_ctx);
instead - fetch after ntohl() we get (b0 << 24) + (b1 << 16) + (b2 << 8)
+ b3,
shift it down by 24 (b0), resuling in b0 * 16 - sizeof(...) both on l-e and
on b-e.

Quoted 7 lines

Quoted 7 lines
PS: when sparse warns you about endianness problems, it might be worth checking
if there really is something wrong. And I don't mean "slap __force cast on it"...
Signed-off-by Al Viro <[email protected]
<mailto:[email protected]>>

Patch applied. Thanks.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt