linux-hardening - Re: [PATCH] crypto: Replace strlcpy with strscpy

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:   Tue, 13 Jun 2023 10:26:35 -0400
From:   Azeem Shaikh <azeemshaikh38@...il.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        linux-hardening@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] crypto: Replace strlcpy with strscpy

On Mon, Jun 12, 2023 at 10:37 PM Eric Biggers <ebiggers@...nel.org> wrote:
>
> On Tue, Jun 13, 2023 at 12:22:58AM +0000, Azeem Shaikh wrote:
> > diff --git a/crypto/lrw.c b/crypto/lrw.c
> > index 1b0f76ba3eb5..bb8c1575645b 100644
> > --- a/crypto/lrw.c
> > +++ b/crypto/lrw.c
> > @@ -359,8 +359,8 @@ static int lrw_create(struct crypto_template *tmpl, struct rtattr **tb)
> >       if (!strncmp(cipher_name, "ecb(", 4)) {
> >               unsigned len;
> >
> > -             len = strlcpy(ecb_name, cipher_name + 4, sizeof(ecb_name));
> > -             if (len < 2 || len >= sizeof(ecb_name))
> > +             len = strscpy(ecb_name, cipher_name + 4, sizeof(ecb_name));
> > +             if (len < 2)
> >                       goto err_free_inst;
>
> This is wrong because 'len' is unsigned.  So the -EFBIG return value is not
> being checked for, so this patch actually removes the overflow check.
>

Thanks for catching this Eric. I can send over a v2 with "unsigned
len" -> "int len" if that works?

> It looks like you've sent over 60 strscpy conversion patches recently
> (https://lore.kernel.org/linux-hardening/?q=f%3Aazeemshaikh38+s%3Astrscpy).
> That's concerning because this is the only one that I happened to review, and it
> introduces a bug.  There are 60 others that could have this same bug.  Can you
> please review all your patches for this same bug?
>

My previous patches replace strlcpy where the return value is ignored.
So this same bug could not have occurred in those. This is the first
set of patches where I'm attempting to replace strlcpy whose return
value is used by callers. For reference below are all my patches where
the return value of strlcpy is used. I have checked that none of these
have the same bug as here.

https://lore.kernel.org/all/20230613004402.3540432-1-azeemshaikh38@gmail.com/
   // Uses int
https://lore.kernel.org/all/20230613004341.3540325-1-azeemshaikh38@gmail.com/
   // Uses int
https://lore.kernel.org/all/20230613004125.3539934-1-azeemshaikh38@gmail.com/
   // Replaces ">= buflen" with "== -E2BIG"
https://lore.kernel.org/all/20230613004054.3539554-1-azeemshaikh38@gmail.com/
   // Uses int