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]>
---
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) <
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...
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 <[email protected]
> <mailto:[email protected]>>
> ---
> 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
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 <[email protected]
> > <mailto:[email protected]>>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt