[PATCH] crypto: Replace strlcpy with strscpy

[PATCH] crypto: Replace strlcpy with strscpy

[Azeem Shaikh]

strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().

Direct replacement is safe here since return value of -E2BIG
is used to check for truncation instead of sizeof(dest).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
 crypto/lrw.c |    4 ++--
 crypto/xts.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

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;
 
 		if (ecb_name[len - 1] != ')')
diff --git a/crypto/xts.c b/crypto/xts.c
index 09be909a6a1a..8a9f9653426e 100644
--- a/crypto/xts.c
+++ b/crypto/xts.c
@@ -398,8 +398,8 @@ static int xts_create(struct crypto_template *tmpl, struct rtattr **tb)
 	if (!strncmp(cipher_name, "ecb(", 4)) {
 		unsigned len;
 
-		len = strlcpy(ctx->name, cipher_name + 4, sizeof(ctx->name));
-		if (len < 2 || len >= sizeof(ctx->name))
+		len = strscpy(ctx->name, cipher_name + 4, sizeof(ctx->name));
+		if (len < 2)
 			goto err_free_inst;
 
 		if (ctx->name[len - 1] != ')')
-- 
2.41.0.162.gfafddb0af9-goog

Re: [PATCH] crypto: Replace strlcpy with strscpy

[Eric Biggers]

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.

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?

- Eric

Re: [PATCH] crypto: Replace strlcpy with strscpy

[Azeem Shaikh]

On Mon, Jun 12, 2023 at 10:37 PM Eric Biggers <ebiggers@kernel.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