linux-kernel - Re: [PATCH 4.19 287/346] crypto: ecdh - avoid unaligned accesses in ecdh_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 2 Jan 2021 12:29:31 +0100
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Pavel Machek <pavel@...x.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "# 3.4.x" <stable@...r.kernel.org>,
        Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH 4.19 287/346] crypto: ecdh - avoid unaligned accesses in ecdh_set_secret()

On Thu, 31 Dec 2020 at 21:09, Pavel Machek <pavel@...x.de> wrote:
>
> Hi!
>
> > ecdh_set_secret() casts a void* pointer to a const u64* in order to
> > feed it into ecc_is_key_valid(). This is not generally permitted by
> > the C standard, and leads to actual misalignment faults on ARMv6
> > cores. In some cases, these are fixed up in software, but this still
> > leads to performance hits that are entirely avoidable.
> >
> > So let's copy the key into the ctx buffer first, which we will do
> > anyway in the common case, and which guarantees correct alignment.
>
> Fair enough... but: params.key_size is validated in
> ecc_is_key_valid(), and that now happens _after_ memcpy.
>
> How is it guaranteed that we don't overflow the buffer during memcpy?
>

It is not, thanks for pointing that out. There are some redundant
checks being performed, so you won't trigger it easily with fuzzing,
but afaict, an intentionally crafted invalid input could indeed
overflow the buffer.

I'll send a fix shortly.

> > +++ b/crypto/ecdh.c
> > @@ -57,12 +57,13 @@ static int ecdh_set_secret(struct crypto
> >               return ecc_gen_privkey(ctx->curve_id, ctx->ndigits,
> >                                      ctx->private_key);
> >
> > -     if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
> > -                          (const u64 *)params.key, params.key_size) < 0)
> > -             return -EINVAL;
> > -
> >       memcpy(ctx->private_key, params.key, params.key_size);
> >
> > +     if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
> > +                          ctx->private_key, params.key_size) < 0) {
> > +             memzero_explicit(ctx->private_key, params.key_size);
> > +             return -EINVAL;
> > +     }
> >       return 0;
>
> Best regards,
>                                                                 Pavel
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany