| Message ID | 20200215061416.GZ23230@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [RFC] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr() | expand |
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 <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
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.
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) <
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> ---