On Wed, Jul 11, 2018 at 03:26:56PM +0800, Herbert Xu wrote:
> On Tue, Jul 10, 2018 at 08:59:05PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@xxxxxxxxxx>
> >
> > It was forgotten to increase DH_KPP_SECRET_MIN_SIZE to include 'q_size',
> > causing an out-of-bounds write of 4 bytes in crypto_dh_encode_key(), and
> > an out-of-bounds read of 4 bytes in crypto_dh_decode_key(). Fix it.
> > Also add a BUG_ON() if crypto_dh_encode_key() doesn't exactly fill the
> > buffer, as that would have found this bug without resorting to KASAN.
> >
> > Reported-by: syzbot+6d38d558c25b53b8f4ed@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Fixes: e3fe0ae12962 ("crypto: dh - add public key verification test")
> > Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> Is it possible to return an error and use WARN_ON instead of BUG_ON?
> Or do the callers not bother to check for errors?
>
The callers do check for errors, but at the point of the proposed BUG_ON() a
buffer overflow may have already occurred, so I think a BUG_ON() would be more
appropriate than a WARN_ON(). Of course, it would be better to prevent any
buffer overflow from occurring in the first place, but that's already the
purpose of the 'len != crypto_dh_key_len(params)' check; the issue was that
'crypto_dh_key_len()' calculated the wrong length.
- Eric
