* [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()
@ 2020-02-15 6:14 Al Viro
2020-02-15 6:29 ` Al Viro
[not found] ` <CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com>
0 siblings, 2 replies; 4+ messages in thread
From: Al Viro @ 2020-02-15 6:14 UTC (permalink / raw)
To: Atul Gupta; +Cc: linux-crypto, linux-kernel
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 <viro@zeniv.linux.org.uk>
---
diff -urN a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
--- 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) <
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()
2020-02-15 6:14 [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr() Al Viro
@ 2020-02-15 6:29 ` Al Viro
[not found] ` <CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com>
1 sibling, 0 replies; 4+ messages in thread
From: Al Viro @ 2020-02-15 6:29 UTC (permalink / raw)
To: Atul Gupta; +Cc: linux-crypto, linux-kernel
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...
^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com>]
* Re: [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()
[not found] ` <CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com>
@ 2020-02-21 5:17 ` Ayush Sawal
2020-02-22 1:44 ` Herbert Xu
0 siblings, 1 reply; 4+ messages in thread
From: Ayush Sawal @ 2020-02-21 5:17 UTC (permalink / raw)
To: viro, Herbert Xu; +Cc: linux-crypto, linux-kernel, vinay.yadav
On 2/15/2020 11:45 AM, Al Viro wrote:
>
> 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 <viro@zeniv.linux.org.uk
> <mailto:viro@zeniv.linux.org.uk>>
> ---
> diff -urN a/drivers/crypto/chelsio/chcr_algo.c
> b/drivers/crypto/chelsio/chcr_algo.c
> --- 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) <
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()
2020-02-21 5:17 ` Ayush Sawal
@ 2020-02-22 1:44 ` Herbert Xu
0 siblings, 0 replies; 4+ messages in thread
From: Herbert Xu @ 2020-02-22 1:44 UTC (permalink / raw)
To: Ayush Sawal; +Cc: viro, linux-crypto, linux-kernel, vinay.yadav
On Fri, Feb 21, 2020 at 10:47:01AM +0530, Ayush Sawal wrote:
> On 2/15/2020 11:45 AM, Al Viro wrote:
>
> >
> > 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 <viro@zeniv.linux.org.uk
> > <mailto:viro@zeniv.linux.org.uk>>
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] 4+ messages in thread
end of thread, other threads:[~2020-02-22 1:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-15 6:14 [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr() Al Viro
2020-02-15 6:29 ` Al Viro
[not found] ` <CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com>
2020-02-21 5:17 ` Ayush Sawal
2020-02-22 1:44 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).