From: Ayush Sawal <ayush.sawal@chelsio.com>
To: viro@zeniv.linux.org.uk, Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
vinay.yadav@chelsio.com
Subject: Re: [RFC][PATCH] almost certain bug in drivers/crypto/chelsio/chcr_algo.c:create_authenc_wr()
Date: Fri, 21 Feb 2020 10:47:01 +0530 [thread overview]
Message-ID: <db4ee9c7-400e-5932-8708-581d91b38385@chelsio.com> (raw)
In-Reply-To: <CAEopUdxRUoMo+uGgiFLWz8NsM1eL7CnkV7gY5PypxrG_nzhNWw@mail.gmail.com>
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
next prev parent reply other threads:[~2020-02-21 5:17 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-02-22 1:44 ` Herbert Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=db4ee9c7-400e-5932-8708-581d91b38385@chelsio.com \
--to=ayush.sawal@chelsio.com \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=vinay.yadav@chelsio.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).