[PATCH V2 0/2] string: Add stracpy and stracpy_pad

[PATCH V2 0/2] string: Add stracpy and stracpy_pad

[Joe Perches]

Add more string copy mechanisms to help avoid defects

Joe Perches (2):
  string: Add stracpy and stracpy_pad mechanisms
  kernel-doc: core-api: Include string.h into core-api

 Documentation/core-api/kernel-api.rst |  3 +++
 include/linux/string.h                | 50 +++++++++++++++++++++++++++++++++--
 lib/string.c                          | 10 ++++---
 3 files changed, 57 insertions(+), 6 deletions(-)

-- 
2.15.0

[PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

Several uses of strlcpy and strscpy have had defects because the
last argument of each function is misused or typoed.

Add macro mechanisms to avoid this defect.

stracpy (copy a string to a string array) must have a string
array as the first argument (dest) and uses sizeof(dest) as the
count of bytes to copy.

These mechanisms verify that the dest argument is an array of
char or other compatible types like u8 or s8 or equivalent.

A BUILD_BUG is emitted when the type of dest is not compatible.

Signed-off-by: Joe Perches <joe@perches.com>
---

V2: Use __same_type testing char[], signed char[], and unsigned char[]
    Rename to, from, and size, dest, src and count
    Correct return of -E2BIG descriptions

 include/linux/string.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 4deb11f7976b..7572cd78cf9f 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -35,6 +35,51 @@ ssize_t strscpy(char *, const char *, size_t);
 /* Wraps calls to strscpy()/memset(), no arch specific code required */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count);
 
+/**
+ * stracpy - Copy a C-string into an array of char/u8/s8 or equivalent
+ * @dest: Where to copy the string, must be an array of char and not a pointer
+ * @src: String to copy, may be a pointer or const char array
+ *
+ * Helper for strscpy().
+ * Copies a maximum of sizeof(@dest) bytes of @src with %NUL termination.
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if @dest is a zero size array or @src was truncated.
+ */
+#define stracpy(dest, src)						\
+({									\
+	size_t count = ARRAY_SIZE(dest);				\
+	BUILD_BUG_ON(!(__same_type(dest, char[]) ||			\
+		       __same_type(dest, unsigned char[]) ||		\
+		       __same_type(dest, signed char[])));		\
+									\
+	strscpy(dest, src, count);					\
+})
+
+/**
+ * stracpy_pad - Copy a C-string into an array of char/u8/s8 with %NUL padding
+ * @dest: Where to copy the string, must be an array of char and not a pointer
+ * @src: String to copy, may be a pointer or const char array
+ *
+ * Helper for strscpy_pad().
+ * Copies a maximum of sizeof(@dest) bytes of @src with %NUL termination
+ * and zero-pads the remaining size of @dest
+ *
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if @dest is a zero size array or @src was truncated.
+ */
+#define stracpy_pad(dest, src)						\
+({									\
+	size_t count = ARRAY_SIZE(dest);				\
+	BUILD_BUG_ON(!(__same_type(dest, char[]) ||			\
+		       __same_type(dest, unsigned char[]) ||		\
+		       __same_type(dest, signed char[])));		\
+									\
+	strscpy_pad(dest, src, count);					\
+})
+
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif
-- 
2.15.0

[PATCH V2 2/2] kernel-doc: core-api: Include string.h into core-api

[Joe Perches]

core-api should show all the various string functions including the
newly added stracpy and stracpy_pad.

Miscellanea:

o Update the Returns: value for strscpy
o fix a defect with %NUL)

Signed-off-by: Joe Perches <joe@perches.com>
---

V2: Correct return of -E2BIG descriptions

 Documentation/core-api/kernel-api.rst |  3 +++
 include/linux/string.h                |  5 +++--
 lib/string.c                          | 10 ++++++----
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index 08af5caf036d..f77de49b1d51 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -42,6 +42,9 @@ String Manipulation
 .. kernel-doc:: lib/string.c
    :export:
 
+.. kernel-doc:: include/linux/string.h
+   :internal:
+
 .. kernel-doc:: mm/util.c
    :functions: kstrdup kstrdup_const kstrndup kmemdup kmemdup_nul memdup_user
                vmemdup_user strndup_user memdup_user_nul
diff --git a/include/linux/string.h b/include/linux/string.h
index 7572cd78cf9f..3cf684db4bc6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -519,8 +519,9 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
  * But this can lead to bugs due to typos, or if prefix is a pointer
  * and not a constant. Instead use str_has_prefix().
  *
- * Returns: 0 if @str does not start with @prefix
-         strlen(@prefix) if @str does start with @prefix
+ * Returns:
+ * * strlen(@prefix) if @str starts with @prefix
+ * * 0 if @str does not start with @prefix
  */
 static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
 {
diff --git a/lib/string.c b/lib/string.c
index 461fb620f85f..f7bc10da4259 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -173,8 +173,9 @@ EXPORT_SYMBOL(strlcpy);
  * doesn't unnecessarily force the tail of the destination buffer to be
  * zeroed.  If zeroing is desired please use strscpy_pad().
  *
- * Return: The number of characters copied (not including the trailing
- *         %NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0 or @src was truncated.
  */
 ssize_t strscpy(char *dest, const char *src, size_t count)
 {
@@ -253,8 +254,9 @@ EXPORT_SYMBOL(strscpy);
  * For full explanation of why you may want to consider using the
  * 'strscpy' functions please see the function docstring for strscpy().
  *
- * Return: The number of characters copied (not including the trailing
- *         %NUL) or -E2BIG if the destination buffer wasn't big enough.
+ * Returns:
+ * * The number of characters copied (not including the trailing %NUL)
+ * * -E2BIG if count is 0 or @src was truncated.
  */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count)
 {
-- 
2.15.0

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 23/07/2019 15.51, Joe Perches wrote:
> Several uses of strlcpy and strscpy have had defects because the
> last argument of each function is misused or typoed.
> 
> Add macro mechanisms to avoid this defect.
> 
> stracpy (copy a string to a string array) must have a string
> array as the first argument (dest) and uses sizeof(dest) as the
> count of bytes to copy.
> 
> These mechanisms verify that the dest argument is an array of
> char or other compatible types like u8 or s8 or equivalent.

Sorry, but "compatible types" has a very specific meaning in C, so
please don't use that word. And yes, the kernel disables -Wpointer-sign,
so passing an u8* or s8* when strscpy() expects a char* is silently
accepted, but does such code exist?

> 
> V2: Use __same_type testing char[], signed char[], and unsigned char[]
>     Rename to, from, and size, dest, src and count

count is just as bad as size in terms of "the expression src might
contain that identifier". But there's actually no reason to even declare
a local variable, just use ARRAY_SIZE() directly as the third argument
to strscpy().

Rasmus

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

On Tue, 2019-07-23 at 16:37 +0200, Rasmus Villemoes wrote:
> On 23/07/2019 15.51, Joe Perches wrote:
> > Several uses of strlcpy and strscpy have had defects because the
> > last argument of each function is misused or typoed.
> > 
> > Add macro mechanisms to avoid this defect.
> > 
> > stracpy (copy a string to a string array) must have a string
> > array as the first argument (dest) and uses sizeof(dest) as the
> > count of bytes to copy.
> > 
> > These mechanisms verify that the dest argument is an array of
> > char or other compatible types like u8 or s8 or equivalent.
> Sorry, but "compatible types" has a very specific meaning in C, so
> please don't use that word.

I think you are being overly pedantic here but
what wording do you actually suggest?

>  And yes, the kernel disables -Wpointer-sign,
> so passing an u8* or s8* when strscpy() expects a char* is silently
> accepted, but does such code exist?

u8 definitely, s8 I'm not sure.

I don't find via grep a use of s8 foo[] = "bar";
or "signed char foo[] = "bar";

I don't think it bad to allow it.

> > V2: Use __same_type testing char[], signed char[], and unsigned char[]
> >     Rename to, from, and size, dest, src and count
> 
> count is just as bad as size in terms of "the expression src might
> contain that identifier". But there's actually no reason to even declare
> a local variable, just use ARRAY_SIZE() directly as the third argument
> to strscpy().

I don't care about that myself.
It's a macro local identifier and shadowing in a macro
is common.  I'm not a big fan of useless underscores.

I think either works.

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 23/07/2019 17.39, Joe Perches wrote:
> On Tue, 2019-07-23 at 16:37 +0200, Rasmus Villemoes wrote:
>> On 23/07/2019 15.51, Joe Perches wrote:
>>>
>>> These mechanisms verify that the dest argument is an array of
>>> char or other compatible types like u8 or s8 or equivalent.
>> Sorry, but "compatible types" has a very specific meaning in C, so
>> please don't use that word.
> 
> I think you are being overly pedantic here but
> what wording do you actually suggest?

I'd just not support anything other than char[], but if you want,
perhaps say "related types", or some other informal description.

>>  And yes, the kernel disables -Wpointer-sign,
>> so passing an u8* or s8* when strscpy() expects a char* is silently
>> accepted, but does such code exist?
> 
> u8 definitely, s8 I'm not sure.

Example (i.e. of someone passing an u8* as destination to some string
copy/formatting function)? I believe you, I'd just like to see the context.

> I don't find via grep a use of s8 foo[] = "bar";
> or "signed char foo[] = "bar";
> 
> I don't think it bad to allow it.

Your patch.

>> count is just as bad as size in terms of "the expression src might
>> contain that identifier". But there's actually no reason to even declare
>> a local variable, just use ARRAY_SIZE() directly as the third argument
>> to strscpy().
> 
> I don't care about that myself.
> It's a macro local identifier and shadowing in a macro
> is common.  I'm not a big fan of useless underscores.

shadowing is not the problem. The identifier "count" appearing in one of
the "dest" or "src" expressions is. For something that's supposed to
help eliminate bugs, such a hidden footgun seems to be a silly thing to
include. No need for some hideous triple-underscore variable, just make
the whole thing

BUILD_BUG_ON(!__same_type())
strscpy(dst, src, ARRAY_SIZE(dst))

Rasmus

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

On Wed, 2019-07-24 at 08:53 +0200, Rasmus Villemoes wrote:
> BUILD_BUG_ON(!__same_type())
> strscpy(dst, src, ARRAY_SIZE(dst))

Doing this without the temporary is less legible to read
the compiler error when someone improperly does:

	char *foo;
	char *bar;

	stracpy(foo, bar);

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Andrew Morton]

On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:

> Several uses of strlcpy and strscpy have had defects because the
> last argument of each function is misused or typoed.
> 
> Add macro mechanisms to avoid this defect.
> 
> stracpy (copy a string to a string array) must have a string
> array as the first argument (dest) and uses sizeof(dest) as the
> count of bytes to copy.
> 
> These mechanisms verify that the dest argument is an array of
> char or other compatible types like u8 or s8 or equivalent.
> 
> A BUILD_BUG is emitted when the type of dest is not compatible.
> 

I'm still reluctant to merge this because we don't have code in -next
which *uses* it.  You did have a patch for that against v1, I believe? 
Please dust it off and send it along?

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Stephen Kitt]

Le 25/09/2019 23:50, Andrew Morton a écrit :
> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
> 
>> Several uses of strlcpy and strscpy have had defects because the
>> last argument of each function is misused or typoed.
>> 
>> Add macro mechanisms to avoid this defect.
>> 
>> stracpy (copy a string to a string array) must have a string
>> array as the first argument (dest) and uses sizeof(dest) as the
>> count of bytes to copy.
>> 
>> These mechanisms verify that the dest argument is an array of
>> char or other compatible types like u8 or s8 or equivalent.
>> 
>> A BUILD_BUG is emitted when the type of dest is not compatible.
>> 
> 
> I'm still reluctant to merge this because we don't have code in -next
> which *uses* it.  You did have a patch for that against v1, I believe?
> Please dust it off and send it along?

Joe had a Coccinelle script to mass-convert strlcpy and strscpy.
Here's a different patch which converts some of ALSA's strcpy calls to
stracpy:


 From 89e9afa562f3351bc000f3aacb1041eafbe0204c Mon Sep 17 00:00:00 2001
 From: Stephen Kitt <steve@sk2.org>
Date: Thu, 26 Sep 2019 01:36:20 +0200
Subject: [PATCH] ALSA: use stracpy in docs, USB and Intel HDMI

This converts the Writing an ALSA driver manual to stracpy instead of
strcpy, and applies the change to the USB drivers and the Intel HDMI
audio driver.

Using stracpy ensures that the target is a char array and that the
copy doesn't overflow (using strscpy).

(This requires Joe Perches' stracpy patch.)

Signed-off-by: Stephen Kitt <steve@sk2.org>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Clemens Ladisch <clemens@ladisch.de>
---
  .../sound/kernel-api/writing-an-alsa-driver.rst  | 16 ++++++++--------
  sound/usb/6fire/chip.c                           |  4 ++--
  sound/usb/6fire/midi.c                           |  2 +-
  sound/usb/6fire/pcm.c                            |  2 +-
  sound/usb/card.c                                 |  2 +-
  sound/usb/line6/driver.c                         |  8 ++++----
  sound/usb/line6/midi.c                           |  4 ++--
  sound/usb/line6/pcm.c                            |  2 +-
  sound/usb/line6/toneport.c                       |  4 ++--
  sound/usb/midi.c                                 |  2 +-
  sound/usb/misc/ua101.c                           |  6 +++---
  sound/usb/mixer.c                                |  2 +-
  sound/usb/stream.c                               |  2 +-
  sound/usb/usx2y/us122l.c                         |  2 +-
  sound/usb/usx2y/usX2Yhwdep.c                     |  2 +-
  sound/usb/usx2y/usbusx2y.c                       |  4 ++--
  sound/x86/intel_hdmi_audio.c                     |  6 +++---
  17 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst 
b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index 132f5eb9b530..90170d853a35 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -321,8 +321,8 @@ to details explained in the following section.
                        goto error;

                /* (4) */
-              strcpy(card->driver, "My Chip");
-              strcpy(card->shortname, "My Own Chip 123");
+              stracpy(card->driver, "My Chip");
+              stracpy(card->shortname, "My Own Chip 123");
                sprintf(card->longname, "%s at 0x%lx irq %i",
                        card->shortname, chip->port, chip->irq);

@@ -434,8 +434,8 @@ Since each component can be properly freed, the 
single

  ::

-  strcpy(card->driver, "My Chip");
-  strcpy(card->shortname, "My Own Chip 123");
+  stracpy(card->driver, "My Chip");
+  stracpy(card->shortname, "My Own Chip 123");
    sprintf(card->longname, "%s at 0x%lx irq %i",
            card->shortname, chip->port, chip->irq);

@@ -1373,7 +1373,7 @@ shows only the skeleton, how to build up the PCM 
interfaces.
                if (err < 0)
                        return err;
                pcm->private_data = chip;
-              strcpy(pcm->name, "My Chip");
+              stracpy(pcm->name, "My Chip");
                chip->pcm = pcm;
                /* set operators */
                snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
@@ -1406,7 +1406,7 @@ function. It would be better to create a 
constructor for pcm, namely,
            if (err < 0)
                    return err;
            pcm->private_data = chip;
-          strcpy(pcm->name, "My Chip");
+          stracpy(pcm->name, "My Chip");
            chip->pcm = pcm;
  	  ....
            return 0;
@@ -2590,7 +2590,7 @@ the string for the currently given item index.
            uinfo->value.enumerated.items = 4;
            if (uinfo->value.enumerated.item > 3)
                    uinfo->value.enumerated.item = 3;
-          strcpy(uinfo->value.enumerated.name,
+          stracpy(uinfo->value.enumerated.name,
                   texts[uinfo->value.enumerated.item]);
            return 0;
    }
@@ -3136,7 +3136,7 @@ function:
    if (err < 0)
            return err;
    rmidi->private_data = chip;
-  strcpy(rmidi->name, "My MIDI");
+  stracpy(rmidi->name, "My MIDI");
    rmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
                        SNDRV_RAWMIDI_INFO_INPUT |
                        SNDRV_RAWMIDI_INFO_DUPLEX;
diff --git a/sound/usb/6fire/chip.c b/sound/usb/6fire/chip.c
index 08c6e6a52eb9..a6e2ca0e1059 100644
--- a/sound/usb/6fire/chip.c
+++ b/sound/usb/6fire/chip.c
@@ -126,8 +126,8 @@ static int usb6fire_chip_probe(struct usb_interface 
*intf,
  		dev_err(&intf->dev, "cannot create alsa card.\n");
  		return ret;
  	}
-	strcpy(card->driver, "6FireUSB");
-	strcpy(card->shortname, "TerraTec DMX6FireUSB");
+	stracpy(card->driver, "6FireUSB");
+	stracpy(card->shortname, "TerraTec DMX6FireUSB");
  	sprintf(card->longname, "%s at %d:%d", card->shortname,
  			device->bus->busnum, device->devnum);

diff --git a/sound/usb/6fire/midi.c b/sound/usb/6fire/midi.c
index de2691d58de6..77700d331b21 100644
--- a/sound/usb/6fire/midi.c
+++ b/sound/usb/6fire/midi.c
@@ -183,7 +183,7 @@ int usb6fire_midi_init(struct sfire_chip *chip)
  		return ret;
  	}
  	rt->instance->private_data = rt;
-	strcpy(rt->instance->name, "DMX6FireUSB MIDI");
+	stracpy(rt->instance->name, "DMX6FireUSB MIDI");
  	rt->instance->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
  			SNDRV_RAWMIDI_INFO_INPUT |
  			SNDRV_RAWMIDI_INFO_DUPLEX;
diff --git a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
index 88ac1c4ee163..70b6f13641f7 100644
--- a/sound/usb/6fire/pcm.c
+++ b/sound/usb/6fire/pcm.c
@@ -656,7 +656,7 @@ int usb6fire_pcm_init(struct sfire_chip *chip)
  	}

  	pcm->private_data = rt;
-	strcpy(pcm->name, "DMX 6Fire USB");
+	stracpy(pcm->name, "DMX 6Fire USB");
  	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &pcm_ops);
  	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &pcm_ops);

diff --git a/sound/usb/card.c b/sound/usb/card.c
index db91dc76cc91..34062d65f030 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -486,7 +486,7 @@ static int snd_usb_audio_create(struct usb_interface 
*intf,

  	card->private_free = snd_usb_audio_free;

-	strcpy(card->driver, "USB-Audio");
+	stracpy(card->driver, "USB-Audio");
  	sprintf(component, "USB%04x:%04x",
  		USB_ID_VENDOR(chip->usb_id), USB_ID_PRODUCT(chip->usb_id));
  	snd_component_add(card, component);
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c
index b5a3f754a4f1..c18dba368551 100644
--- a/sound/usb/line6/driver.c
+++ b/sound/usb/line6/driver.c
@@ -663,7 +663,7 @@ static int line6_hwdep_init(struct usb_line6 *line6)
  	err = snd_hwdep_new(line6->card, "config", 0, &hwdep);
  	if (err < 0)
  		goto end;
-	strcpy(hwdep->name, "config");
+	stracpy(hwdep->name, "config");
  	hwdep->iface = SNDRV_HWDEP_IFACE_LINE6;
  	hwdep->ops = hwdep_ops;
  	hwdep->private_data = line6;
@@ -751,9 +751,9 @@ int line6_probe(struct usb_interface *interface,
  	line6->ifcdev = &interface->dev;
  	INIT_DELAYED_WORK(&line6->startup_work, line6_startup_work);

-	strcpy(card->id, properties->id);
-	strcpy(card->driver, driver_name);
-	strcpy(card->shortname, properties->name);
+	stracpy(card->id, properties->id);
+	stracpy(card->driver, driver_name);
+	stracpy(card->shortname, properties->name);
  	sprintf(card->longname, "Line 6 %s at USB %s", properties->name,
  		dev_name(line6->ifcdev));
  	card->private_free = line6_destruct;
diff --git a/sound/usb/line6/midi.c b/sound/usb/line6/midi.c
index ba0e2b7e8fe1..fd1c2248c2ef 100644
--- a/sound/usb/line6/midi.c
+++ b/sound/usb/line6/midi.c
@@ -226,8 +226,8 @@ static int snd_line6_new_midi(struct usb_line6 
*line6,
  		return err;

  	rmidi = *rmidi_ret;
-	strcpy(rmidi->id, line6->properties->id);
-	strcpy(rmidi->name, line6->properties->name);
+	stracpy(rmidi->id, line6->properties->id);
+	stracpy(rmidi->name, line6->properties->name);

  	rmidi->info_flags =
  	    SNDRV_RAWMIDI_INFO_OUTPUT |
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c
index f70211e6b174..66cebbc0d18a 100644
--- a/sound/usb/line6/pcm.c
+++ b/sound/usb/line6/pcm.c
@@ -493,7 +493,7 @@ static int snd_line6_new_pcm(struct usb_line6 
*line6, struct snd_pcm **pcm_ret)
  	if (err < 0)
  		return err;
  	pcm = *pcm_ret;
-	strcpy(pcm->name, line6->properties->name);
+	stracpy(pcm->name, line6->properties->name);

  	/* set operators */
  	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
diff --git a/sound/usb/line6/toneport.c b/sound/usb/line6/toneport.c
index d0a555dbe324..ec704485861c 100644
--- a/sound/usb/line6/toneport.c
+++ b/sound/usb/line6/toneport.c
@@ -198,8 +198,8 @@ static int snd_toneport_source_info(struct 
snd_kcontrol *kcontrol,
  	if (uinfo->value.enumerated.item >= size)
  		uinfo->value.enumerated.item = size - 1;

-	strcpy(uinfo->value.enumerated.name,
-	       toneport_source_info[uinfo->value.enumerated.item].name);
+	stracpy(uinfo->value.enumerated.name,
+		toneport_source_info[uinfo->value.enumerated.item].name);

  	return 0;
  }
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index b737f0ec77d0..4e8ec3a6db6f 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -2245,7 +2245,7 @@ static int snd_usbmidi_create_rawmidi(struct 
snd_usb_midi *umidi,
  			      out_ports, in_ports, &rmidi);
  	if (err < 0)
  		return err;
-	strcpy(rmidi->name, umidi->card->shortname);
+	stracpy(rmidi->name, umidi->card->shortname);
  	rmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
  			    SNDRV_RAWMIDI_INFO_INPUT |
  			    SNDRV_RAWMIDI_INFO_DUPLEX;
diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
index 307b72d5fffa..37531aa55450 100644
--- a/sound/usb/misc/ua101.c
+++ b/sound/usb/misc/ua101.c
@@ -1267,8 +1267,8 @@ static int ua101_probe(struct usb_interface 
*interface,
  		goto probe_error;

  	name = usb_id->idProduct == 0x0044 ? "UA-1000" : "UA-101";
-	strcpy(card->driver, "UA-101");
-	strcpy(card->shortname, name);
+	stracpy(card->driver, "UA-101");
+	stracpy(card->shortname, name);
  	usb_make_path(ua->dev, usb_path, sizeof(usb_path));
  	snprintf(ua->card->longname, sizeof(ua->card->longname),
  		 "EDIROL %s (serial %s), %u Hz at %s, %s speed", name,
@@ -1293,7 +1293,7 @@ static int ua101_probe(struct usb_interface 
*interface,
  	if (err < 0)
  		goto probe_error;
  	ua->pcm->private_data = ua;
-	strcpy(ua->pcm->name, name);
+	stracpy(ua->pcm->name, name);
  	snd_pcm_set_ops(ua->pcm, SNDRV_PCM_STREAM_PLAYBACK, 
&playback_pcm_ops);
  	snd_pcm_set_ops(ua->pcm, SNDRV_PCM_STREAM_CAPTURE, &capture_pcm_ops);

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 3fd1d1749edf..3d123163c78e 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -3408,7 +3408,7 @@ int snd_usb_create_mixer(struct snd_usb_audio 
*chip, int ctrlif,
  	struct usb_mixer_interface *mixer;
  	int err;

-	strcpy(chip->card->mixername, "USB Mixer");
+	stracpy(chip->card->mixername, "USB Mixer");

  	mixer = kzalloc(sizeof(*mixer), GFP_KERNEL);
  	if (!mixer)
diff --git a/sound/usb/stream.c b/sound/usb/stream.c
index 11785f9652ad..1969763c88db 100644
--- a/sound/usb/stream.c
+++ b/sound/usb/stream.c
@@ -531,7 +531,7 @@ static int __snd_usb_add_audio_stream(struct 
snd_usb_audio *chip,
  	if (chip->pcm_devs > 0)
  		sprintf(pcm->name, "USB Audio #%d", chip->pcm_devs);
  	else
-		strcpy(pcm->name, "USB Audio");
+		stracpy(pcm->name, "USB Audio");

  	snd_usb_init_substream(as, stream, fp, pd);

diff --git a/sound/usb/usx2y/us122l.c b/sound/usb/usx2y/us122l.c
index e82c5236482d..c5f09b382e2b 100644
--- a/sound/usb/usx2y/us122l.c
+++ b/sound/usb/usx2y/us122l.c
@@ -539,7 +539,7 @@ static int usx2y_create_card(struct usb_device 
*device,
  	init_waitqueue_head(&US122L(card)->sk.sleep);
  	US122L(card)->is_us144 = flags & US122L_FLAG_US144;
  	INIT_LIST_HEAD(&US122L(card)->midi_list);
-	strcpy(card->driver, "USB "NAME_ALLCAPS"");
+	stracpy(card->driver, "USB "NAME_ALLCAPS"");
  	sprintf(card->shortname, "TASCAM "NAME_ALLCAPS"");
  	sprintf(card->longname, "%s (%x:%x if %d at %03d/%03d)",
  		card->shortname,
diff --git a/sound/usb/usx2y/usX2Yhwdep.c b/sound/usb/usx2y/usX2Yhwdep.c
index d1caa8ed9e68..72ca8dba0f5b 100644
--- a/sound/usb/usx2y/usX2Yhwdep.c
+++ b/sound/usb/usx2y/usX2Yhwdep.c
@@ -115,7 +115,7 @@ static int snd_usX2Y_hwdep_dsp_status(struct 
snd_hwdep *hw,
  	}
  	if (0 > id)
  		return -ENODEV;
-	strcpy(info->id, type_ids[id]);
+	stracpy(info->id, type_ids[id]);
  	info->num_dsps = 2;		// 0: Prepad Data, 1: FPGA Code
  	if (us428->chip_status & USX2Y_STAT_CHIP_INIT)
  		info->chip_ready = 1;
diff --git a/sound/usb/usx2y/usbusx2y.c b/sound/usb/usx2y/usbusx2y.c
index c54158146917..b0b94d2884f4 100644
--- a/sound/usb/usx2y/usbusx2y.c
+++ b/sound/usb/usx2y/usbusx2y.c
@@ -347,8 +347,8 @@ static int usX2Y_create_card(struct usb_device 
*device,
  	init_waitqueue_head(&usX2Y(card)->prepare_wait_queue);
  	mutex_init(&usX2Y(card)->pcm_mutex);
  	INIT_LIST_HEAD(&usX2Y(card)->midi_list);
-	strcpy(card->driver, "USB "NAME_ALLCAPS"");
-	sprintf(card->shortname, "TASCAM "NAME_ALLCAPS"");
+	stracpy(card->driver, "USB "NAME_ALLCAPS"");
+	stracpy(card->shortname, "TASCAM "NAME_ALLCAPS"");
  	sprintf(card->longname, "%s (%x:%x if %d at %03d/%03d)",
  		card->shortname,
  		le16_to_cpu(device->descriptor.idVendor),
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
index 5fd4e32247a6..f312e382e3e0 100644
--- a/sound/x86/intel_hdmi_audio.c
+++ b/sound/x86/intel_hdmi_audio.c
@@ -1728,9 +1728,9 @@ static int hdmi_lpe_audio_probe(struct 
platform_device *pdev)
  	card_ctx = card->private_data;
  	card_ctx->dev = &pdev->dev;
  	card_ctx->card = card;
-	strcpy(card->driver, INTEL_HAD);
-	strcpy(card->shortname, "Intel HDMI/DP LPE Audio");
-	strcpy(card->longname, "Intel HDMI/DP LPE Audio");
+	stracpy(card->driver, INTEL_HAD);
+	stracpy(card->shortname, "Intel HDMI/DP LPE Audio");
+	stracpy(card->longname, "Intel HDMI/DP LPE Audio");

  	card_ctx->irq = -1;

-- 
2.20.1


(And yes, there's more to fix in the string handling here; I focused
on stracpy only.)

Regards,

Stephen

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 26/09/2019 02.01, Stephen Kitt wrote:
> Le 25/09/2019 23:50, Andrew Morton a écrit :
>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
>>
>>> Several uses of strlcpy and strscpy have had defects because the
>>> last argument of each function is misused or typoed.
>>>
>>> Add macro mechanisms to avoid this defect.
>>>
>>> stracpy (copy a string to a string array) must have a string
>>> array as the first argument (dest) and uses sizeof(dest) as the
>>> count of bytes to copy.
>>>
>>> These mechanisms verify that the dest argument is an array of
>>> char or other compatible types like u8 or s8 or equivalent.
>>>
>>> A BUILD_BUG is emitted when the type of dest is not compatible.
>>>
>>
>> I'm still reluctant to merge this because we don't have code in -next
>> which *uses* it.  You did have a patch for that against v1, I believe?
>> Please dust it off and send it along?
> 
> Joe had a Coccinelle script to mass-convert strlcpy and strscpy.
> Here's a different patch which converts some of ALSA's strcpy calls to
> stracpy:

Please don't. At least not for the cases where the source is a string
literal - that just gives worse code generation (because gcc doesn't
know anything about strscpy or strlcpy), and while a run-time (silent)
truncation is better than a run-time buffer overflow, wouldn't it be
even better with a build time error?

Something like

/** static_strcpy - run-time version of static initialization
 *
 * @d: destination array, must be array of char of known size
 * @s: source, must be a string literal
 *
 * This is a simple wrapper for strcpy(d, s), which checks at build-time
that the strcpy() won't overflow. In most cases (for short strings), gcc
won't even emit a call to strcpy but rather just do a few immediate
stores into the destination, e.g.

   0:   c7 07 66 6f 6f 00       movl   $0x6f6f66,(%rdi)

 * for strcpy(d->name, "foo").
 */

#define static_strcpy(d, s) ({ \
  static_assert(__same_type(d, char[]), "destination must be char array"); \
  static_assert(__same_type(s, const char[], "source must be a string
literal"); \
  static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
  strcpy(d, s); \
})

The "" s "" trick guarantees that s is a string literal - it probably
doesn't give a very nice error message, but that's why I added the
redundant type comparison to a const char[] which should hopefully give
a better clue.

Rasmus

PS: Yes, we already have a fortified strcpy(), but for some reason it
doesn't catch the common case where we're populating a char[] member of
some struct. So this

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e78017a3e1bd..3b0c5ae9f49e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3420,3 +3420,14 @@ int sscanf(const char *buf, const char *fmt, ...)
        return i;
 }
 EXPORT_SYMBOL(sscanf);
+
+struct s { char name[4]; };
+char buf[4];
+void f3(void)
+{
+       strcpy(buf, "long");
+}
+void f4(struct s *s)
+{
+       strcpy(s->name, "long");
+}

with CONFIG_FORTIFY_SOURCE=y complains about f3(), but f4() is just fine...

PPS: A quick test of static_strcpy():

// ss.c
#include <errno.h>
#include <string.h>
#include <assert.h>

#define __same_type(x, y) __builtin_types_compatible_p(typeof(x), typeof(y))

#define static_strcpy(d, s) ({						\
  static_assert(__same_type(d, char[]), "destination must be char array"); \
  static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
  static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
  strcpy(d, s); \
})

struct s { char name[4]; };

void f0(struct s *s) { static_strcpy(s->name, "foo"); }
#if 1
void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); }
void f2(struct s *s) { static_strcpy(s->name, "long"); }
void f3(char *d) { static_strcpy(d, "foo"); }
void f4(char *d) { static_strcpy(d, strerror(EIO)); }
#endif

// gcc -Wall -O2 -o ss.o -c ss.c
In file included from ss.c:3:0:
ss.c: In function ‘f1’:
ss.c:9:3: error: static assertion failed: "source must be a string literal"
   static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
   ^
ss.c:18:24: note: in expansion of macro ‘static_strcpy’
 void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); }
                        ^~~~~~~~~~~~~
ss.c:18:47: error: expected ‘)’ before ‘strerror’
 void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); }
                                               ^
ss.c:10:40: note: in definition of macro ‘static_strcpy’
   static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
                                        ^
In file included from ss.c:3:0:
ss.c: In function ‘f2’:
ss.c:10:3: error: static assertion failed: "source does not fit in
destination"
   static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
   ^
ss.c:19:24: note: in expansion of macro ‘static_strcpy’
 void f2(struct s *s) { static_strcpy(s->name, "long"); }
                        ^~~~~~~~~~~~~
ss.c: In function ‘f3’:
ss.c:8:3: error: static assertion failed: "destination must be char array"
   static_assert(__same_type(d, char[]), "destination must be char
array"); \
   ^
ss.c:20:20: note: in expansion of macro ‘static_strcpy’
 void f3(char *d) { static_strcpy(d, "foo"); }
                    ^~~~~~~~~~~~~
ss.c: In function ‘f4’:
ss.c:8:3: error: static assertion failed: "destination must be char array"
   static_assert(__same_type(d, char[]), "destination must be char
array"); \
   ^
ss.c:21:20: note: in expansion of macro ‘static_strcpy’
 void f4(char *d) { static_strcpy(d, strerror(EIO)); }
                    ^~~~~~~~~~~~~
ss.c:9:3: error: static assertion failed: "source must be a string literal"
   static_assert(__same_type(s, const char[]), "source must be a string
literal"); \
   ^
ss.c:21:20: note: in expansion of macro ‘static_strcpy’
 void f4(char *d) { static_strcpy(d, strerror(EIO)); }
                    ^~~~~~~~~~~~~
ss.c:21:37: error: expected ‘)’ before ‘strerror’
 void f4(char *d) { static_strcpy(d, strerror(EIO)); }
                                     ^
ss.c:10:40: note: in definition of macro ‘static_strcpy’
   static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in
destination"); \
                                        ^

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Stephen Kitt]

Le 26/09/2019 09:29, Rasmus Villemoes a écrit :
> On 26/09/2019 02.01, Stephen Kitt wrote:
>> Le 25/09/2019 23:50, Andrew Morton a écrit :
>>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> 
>>> wrote:
>>> 
>>>> Several uses of strlcpy and strscpy have had defects because the
>>>> last argument of each function is misused or typoed.
>>>> 
>>>> Add macro mechanisms to avoid this defect.
>>>> 
>>>> stracpy (copy a string to a string array) must have a string
>>>> array as the first argument (dest) and uses sizeof(dest) as the
>>>> count of bytes to copy.
>>>> 
>>>> These mechanisms verify that the dest argument is an array of
>>>> char or other compatible types like u8 or s8 or equivalent.
>>>> 
>>>> A BUILD_BUG is emitted when the type of dest is not compatible.
>>>> 
>>> 
>>> I'm still reluctant to merge this because we don't have code in -next
>>> which *uses* it.  You did have a patch for that against v1, I 
>>> believe?
>>> Please dust it off and send it along?
>> 
>> Joe had a Coccinelle script to mass-convert strlcpy and strscpy.
>> Here's a different patch which converts some of ALSA's strcpy calls to
>> stracpy:
> 
> Please don't. At least not for the cases where the source is a string
> literal - that just gives worse code generation (because gcc doesn't
> know anything about strscpy or strlcpy), and while a run-time (silent)
> truncation is better than a run-time buffer overflow, wouldn't it be
> even better with a build time error?

Yes, that was the plan once Joe's patch gets merged (if it does), and my
patch was only an example of using stracpy, as a step on the road. I was
intending to follow up with a patch converting stracpy to something like
https://www.openwall.com/lists/kernel-hardening/2019/07/06/14

__FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t 
count)
{
	size_t dest_size = __builtin_object_size(dest, 0);
	size_t src_size = __builtin_object_size(src, 0);
	if (__builtin_constant_p(count) &&
	    __builtin_constant_p(src_size) &&
	    __builtin_constant_p(dest_size) &&
	    src_size <= count &&
	    src_size <= dest_size &&
	    src[src_size - 1] == '\0') {
		strcpy(dest, src);
		return src_size - 1;
	} else {
		return __strscpy(dest, src, count);
	}
}

which, as a macro, would become

#define stracpy(dest, src)						\
({									\
	size_t count = ARRAY_SIZE(dest);				\
	size_t dest_size = __builtin_object_size(dest, 0);		\
	size_t src_size = __builtin_object_size(src, 0);		\
	BUILD_BUG_ON(!(__same_type(dest, char[]) ||			\
		       __same_type(dest, unsigned char[]) ||		\
		       __same_type(dest, signed char[])));		\
									\
	(__builtin_constant_p(count) &&					\
	 __builtin_constant_p(src_size) &&				\
	 __builtin_constant_p(dest_size) &&				\
	 src_size <= count &&						\
	 src_size <= dest_size &&					\
	 src[src_size - 1] == '\0') ?					\
		(((size_t) strcpy(dest, src)) & 0) + src_size - 1	\
	:								\
		strscpy(dest, src, count);				\
})

and both of these get optimised to movs when copying a constant string
which fits in the target.

I was going at this from the angle of improving the existing APIs and
their resulting code. But I like your approach of failing at compile
time.

Perhaps we could do both ;-).

Regards,

Stephen

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Joe Perches]

On Wed, 2019-09-25 at 14:50 -0700, Andrew Morton wrote:
> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > Several uses of strlcpy and strscpy have had defects because the
> > last argument of each function is misused or typoed.
> > 
> > Add macro mechanisms to avoid this defect.
> > 
> > stracpy (copy a string to a string array) must have a string
> > array as the first argument (dest) and uses sizeof(dest) as the
> > count of bytes to copy.
> > 
> > These mechanisms verify that the dest argument is an array of
> > char or other compatible types like u8 or s8 or equivalent.
> > 
> > A BUILD_BUG is emitted when the type of dest is not compatible.
> > 
> 
> I'm still reluctant to merge this because we don't have code in -next
> which *uses* it.  You did have a patch for that against v1, I believe? 
> Please dust it off and send it along?

https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

I gave up, especially after the snark from Linus
where he wrote I don't understand this stuff.

He's just too full of himself here merely using
argument from authority.

Creating and using a function like copy_string with
both source and destination lengths specified is
is also potentially a large source of defects where
the stracpy macro atop strscpy does not have a
defect path other than the src not being a string
at all.

I think the analysis of defects in string function
in the kernel is overly difficult today given the
number of possible uses of pointer and length in
strcpy/strncpy/strlcpy/stracpy.

I think also that there is some sense in what he
wrote against the "word salad" use of str<foo>cpy,
but using stracpy as a macro when possible instead
of strscpy also makes the analysis of defects rather
simpler.

The trivial script cocci I posted works well for the
simple cases.

https://lore.kernel.org/cocci/66fcdbf607d7d0bea41edb39e5579d63b62b7d84.camel@perches.com/

The more complicated cocci script Julia posted is
still not quite correct as it required intermediate
compilation for verification of specified lengths.

https://lkml.org/lkml/2019/7/25/1406

Tell me again if you still want it and maybe the
couple conversions that mm/ would get.

via:

$ spatch --all-includes --in-place -sp-file str.cpy.cocci mm
$ git diff --stat -p mm
--
 mm/dmapool.c | 2 +-
 mm/zswap.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index fe5d33060415..b3a4feb423f8 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -158,7 +158,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	if (!retval)
 		return retval;
 
-	strlcpy(retval->name, name, sizeof(retval->name));
+	stracpy(retval->name, name);
 
 	retval->dev = dev;
 
diff --git a/mm/zswap.c b/mm/zswap.c
index 08b6cefae5d8..c6cd38de185a 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -533,7 +533,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
 	}
 	pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
 
-	strlcpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
+	stracpy(pool->tfm_name, compressor);
 	pool->tfm = alloc_percpu(struct crypto_comp *);
 	if (!pool->tfm) {
 		pr_err("percpu alloc failed\n");

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Rasmus Villemoes]

On 26/09/2019 10.25, Stephen Kitt wrote:
> Le 26/09/2019 09:29, Rasmus Villemoes a écrit :
>> On 26/09/2019 02.01, Stephen Kitt wrote:
>>> Le 25/09/2019 23:50, Andrew Morton a écrit :
>>>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
>>>>
>>
>> Please don't. At least not for the cases where the source is a string
>> literal - that just gives worse code generation (because gcc doesn't
>> know anything about strscpy or strlcpy), and while a run-time (silent)
>> truncation is better than a run-time buffer overflow, wouldn't it be
>> even better with a build time error?
> 
> Yes, that was the plan once Joe's patch gets merged (if it does), and my
> patch was only an example of using stracpy, as a step on the road. I was
> intending to follow up with a patch converting stracpy to something like
> https://www.openwall.com/lists/kernel-hardening/2019/07/06/14
> 
> __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
> {
>     size_t dest_size = __builtin_object_size(dest, 0);
>     size_t src_size = __builtin_object_size(src, 0);
>     if (__builtin_constant_p(count) &&
>         __builtin_constant_p(src_size) &&
>         __builtin_constant_p(dest_size) &&

Eh? Isn't the output of __builtin_object_size() by definition a
compile-time constant - whatever the compiler happens to know about the
object size (or a sentinel 0 or -1 depending on the type argument)?

> 
> #define stracpy(dest, src)                        \
> ({                                    \
>     size_t count = ARRAY_SIZE(dest);                \
>     size_t dest_size = __builtin_object_size(dest, 0);        \
>     size_t src_size = __builtin_object_size(src, 0);        \
>     BUILD_BUG_ON(!(__same_type(dest, char[]) ||            \
>                __same_type(dest, unsigned char[]) ||        \
>                __same_type(dest, signed char[])));        \
>                                     \
>     (__builtin_constant_p(count) &&                    \
>      __builtin_constant_p(src_size) &&                \
>      __builtin_constant_p(dest_size) &&                \
>      src_size <= count &&                        \
>      src_size <= dest_size &&                    \
>      src[src_size - 1] == '\0') ?                    \
>         (((size_t) strcpy(dest, src)) & 0) + src_size - 1    \
>     :                                \
>         strscpy(dest, src, count);                \
> })
> 
> and both of these get optimised to movs when copying a constant string
> which fits in the target.

But does it catch the case of overflowing a char[] member in a struct
passed by reference at build time? I'm surprised that
__builtin_object_size(dest, 0) seems to be (size_t)-1, when dest is
s->name (with struct s { char name[4]; };). So I'm not very confident
that any of the fancy fortify logic actually works here - we _really_
should have some Kbuild infrastructure for saying "this .c file should
not compile" so we can test that the fortifications actually work in the
simple and common cases.

> I was going at this from the angle of improving the existing APIs and
> their resulting code.

I'm not against stracpy() as a wrapper for strscpy(), but we should make
sure that whenever we can fail at build time (i.e., both source and dst
lengths known), we do - and in that case also let the compiler optimize
the copy (not only to do the immediate movs, but that also gives it
wider opportunity to remove it completely as dead stores if the
surrounding code ends up dead - with a call to some strscpy(), gcc
cannot eliminate that). If stracpy() can be made sufficiently magic that
it fails at build time for the string literal cases, fine, let's not add
yet another API. Otherwise, I think the static_strcpy() is a much more
readable and reliable API for those cases.

If I'm reading your stracpy() macro correctly, you're explicitly
requesting a run-time truncation (the src_size <= dest_size check
causing as to fall back to strscpy) rather than failing at build time.

Rasmus

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Kees Cook]

On Thu, Sep 26, 2019 at 01:34:36AM -0700, Joe Perches wrote:
> On Wed, 2019-09-25 at 14:50 -0700, Andrew Morton wrote:
> > On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > Several uses of strlcpy and strscpy have had defects because the
> > > last argument of each function is misused or typoed.
> > > 
> > > Add macro mechanisms to avoid this defect.
> > > 
> > > stracpy (copy a string to a string array) must have a string
> > > array as the first argument (dest) and uses sizeof(dest) as the
> > > count of bytes to copy.
> > > 
> > > These mechanisms verify that the dest argument is an array of
> > > char or other compatible types like u8 or s8 or equivalent.
> > > 
> > > A BUILD_BUG is emitted when the type of dest is not compatible.
> > > 
> > 
> > I'm still reluctant to merge this because we don't have code in -next
> > which *uses* it.  You did have a patch for that against v1, I believe? 
> > Please dust it off and send it along?
> 
> https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
> 
> I gave up, especially after the snark from Linus
> where he wrote I don't understand this stuff.
> 
> He's just too full of himself here merely using
> argument from authority.
> 
> Creating and using a function like copy_string with
> both source and destination lengths specified is
> is also potentially a large source of defects where
> the stracpy macro atop strscpy does not have a
> defect path other than the src not being a string
> at all.
> 
> I think the analysis of defects in string function
> in the kernel is overly difficult today given the
> number of possible uses of pointer and length in
> strcpy/strncpy/strlcpy/stracpy.
> 
> I think also that there is some sense in what he
> wrote against the "word salad" use of str<foo>cpy,
> but using stracpy as a macro when possible instead
> of strscpy also makes the analysis of defects rather
> simpler.
> 
> The trivial script cocci I posted works well for the
> simple cases.
> 
> https://lore.kernel.org/cocci/66fcdbf607d7d0bea41edb39e5579d63b62b7d84.camel@perches.com/
> 
> The more complicated cocci script Julia posted is
> still not quite correct as it required intermediate
> compilation for verification of specified lengths.
> 
> https://lkml.org/lkml/2019/7/25/1406
> 
> Tell me again if you still want it and maybe the
> couple conversions that mm/ would get.

FWIW, I want it because it creates a compile-time-verifiable API for a
non-trivial set of common string copying flaws.

CONFIG_FORTIFY_SOURCE isn't able to handle these (yet?) because it's
examining the outer size of structures that hold char arrays. And even
if we built in the logic to deal with it correctly, it'd still split
the detection between compile time and run time.

-Kees

-- 
Kees Cook

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Julia Lawall]

On Thu, 26 Sep 2019, Joe Perches wrote:

> On Wed, 2019-09-25 at 14:50 -0700, Andrew Morton wrote:
> > On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
> >
> > > Several uses of strlcpy and strscpy have had defects because the
> > > last argument of each function is misused or typoed.
> > >
> > > Add macro mechanisms to avoid this defect.
> > >
> > > stracpy (copy a string to a string array) must have a string
> > > array as the first argument (dest) and uses sizeof(dest) as the
> > > count of bytes to copy.
> > >
> > > These mechanisms verify that the dest argument is an array of
> > > char or other compatible types like u8 or s8 or equivalent.
> > >
> > > A BUILD_BUG is emitted when the type of dest is not compatible.
> > >
> >
> > I'm still reluctant to merge this because we don't have code in -next
> > which *uses* it.  You did have a patch for that against v1, I believe?
> > Please dust it off and send it along?
>
> https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
>
> I gave up, especially after the snark from Linus
> where he wrote I don't understand this stuff.
>
> He's just too full of himself here merely using
> argument from authority.
>
> Creating and using a function like copy_string with
> both source and destination lengths specified is
> is also potentially a large source of defects where
> the stracpy macro atop strscpy does not have a
> defect path other than the src not being a string
> at all.
>
> I think the analysis of defects in string function
> in the kernel is overly difficult today given the
> number of possible uses of pointer and length in
> strcpy/strncpy/strlcpy/stracpy.
>
> I think also that there is some sense in what he
> wrote against the "word salad" use of str<foo>cpy,
> but using stracpy as a macro when possible instead
> of strscpy also makes the analysis of defects rather
> simpler.
>
> The trivial script cocci I posted works well for the
> simple cases.
>
> https://lore.kernel.org/cocci/66fcdbf607d7d0bea41edb39e5579d63b62b7d84.camel@perches.com/
>
> The more complicated cocci script Julia posted is
> still not quite correct as it required intermediate
> compilation for verification of specified lengths.

The problem seems to be detecting whether the string can reach user level
and knowing whether padding is needed.  There are many cases where the
copied string is a constant and can easily be checked to fit into the
destination.  But without further investigation that I am not able to do
at the moment, it's not clear how to address the user level issue.

julia

>
> https://lkml.org/lkml/2019/7/25/1406
>
> Tell me again if you still want it and maybe the
> couple conversions that mm/ would get.
>
> via:
>
> $ spatch --all-includes --in-place -sp-file str.cpy.cocci mm
> $ git diff --stat -p mm
> --
>  mm/dmapool.c | 2 +-
>  mm/zswap.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index fe5d33060415..b3a4feb423f8 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -158,7 +158,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	if (!retval)
>  		return retval;
>
> -	strlcpy(retval->name, name, sizeof(retval->name));
> +	stracpy(retval->name, name);
>
>  	retval->dev = dev;
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 08b6cefae5d8..c6cd38de185a 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -533,7 +533,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  	}
>  	pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
>
> -	strlcpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
> +	stracpy(pool->tfm_name, compressor);
>  	pool->tfm = alloc_percpu(struct crypto_comp *);
>  	if (!pool->tfm) {
>  		pr_err("percpu alloc failed\n");
>
>
>
>

Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms

[Julia Lawall]

On Thu, 26 Sep 2019, Joe Perches wrote:

> On Wed, 2019-09-25 at 14:50 -0700, Andrew Morton wrote:
> > On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com> wrote:
> >
> > > Several uses of strlcpy and strscpy have had defects because the
> > > last argument of each function is misused or typoed.
> > >
> > > Add macro mechanisms to avoid this defect.
> > >
> > > stracpy (copy a string to a string array) must have a string
> > > array as the first argument (dest) and uses sizeof(dest) as the
> > > count of bytes to copy.
> > >
> > > These mechanisms verify that the dest argument is an array of
> > > char or other compatible types like u8 or s8 or equivalent.
> > >
> > > A BUILD_BUG is emitted when the type of dest is not compatible.
> > >
> >
> > I'm still reluctant to merge this because we don't have code in -next
> > which *uses* it.  You did have a patch for that against v1, I believe?
> > Please dust it off and send it along?
>
> https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
>
> I gave up, especially after the snark from Linus
> where he wrote I don't understand this stuff.
>
> He's just too full of himself here merely using
> argument from authority.
>
> Creating and using a function like copy_string with
> both source and destination lengths specified is
> is also potentially a large source of defects where
> the stracpy macro atop strscpy does not have a
> defect path other than the src not being a string
> at all.
>
> I think the analysis of defects in string function
> in the kernel is overly difficult today given the
> number of possible uses of pointer and length in
> strcpy/strncpy/strlcpy/stracpy.
>
> I think also that there is some sense in what he
> wrote against the "word salad" use of str<foo>cpy,
> but using stracpy as a macro when possible instead
> of strscpy also makes the analysis of defects rather
> simpler.
>
> The trivial script cocci I posted works well for the
> simple cases.
>
> https://lore.kernel.org/cocci/66fcdbf607d7d0bea41edb39e5579d63b62b7d84.camel@perches.com/
>
> The more complicated cocci script Julia posted is
> still not quite correct as it required intermediate
> compilation for verification of specified lengths.

The script works fine without compilation, but uses compilation as an
extra sanity check.  When there is only one possible declaration of a
given buffer, then the compilation is not really needed.

julia

>
> https://lkml.org/lkml/2019/7/25/1406
>
> Tell me again if you still want it and maybe the
> couple conversions that mm/ would get.
>
> via:
>
> $ spatch --all-includes --in-place -sp-file str.cpy.cocci mm
> $ git diff --stat -p mm
> --
>  mm/dmapool.c | 2 +-
>  mm/zswap.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/dmapool.c b/mm/dmapool.c
> index fe5d33060415..b3a4feb423f8 100644
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -158,7 +158,7 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>  	if (!retval)
>  		return retval;
>
> -	strlcpy(retval->name, name, sizeof(retval->name));
> +	stracpy(retval->name, name);
>
>  	retval->dev = dev;
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 08b6cefae5d8..c6cd38de185a 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -533,7 +533,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  	}
>  	pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
>
> -	strlcpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
> +	stracpy(pool->tfm_name, compressor);
>  	pool->tfm = alloc_percpu(struct crypto_comp *);
>  	if (!pool->tfm) {
>  		pr_err("percpu alloc failed\n");
>
>
>
>