* [PATCH V2 0/2] string: Add stracpy and stracpy_pad @ 2019-07-23 13:51 Joe Perches 2019-07-23 13:51 ` [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms Joe Perches 2019-07-23 13:51 ` [PATCH V2 2/2] kernel-doc: core-api: Include string.h into core-api Joe Perches 0 siblings, 2 replies; 16+ messages in thread From: Joe Perches @ 2019-07-23 13:51 UTC (permalink / raw) To: Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes, Andrew Morton, linux-doc 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-07-23 13:51 [PATCH V2 0/2] string: Add stracpy and stracpy_pad Joe Perches @ 2019-07-23 13:51 ` Joe Perches 2019-07-23 14:37 ` Rasmus Villemoes 2019-09-25 21:50 ` Andrew Morton 2019-07-23 13:51 ` [PATCH V2 2/2] kernel-doc: core-api: Include string.h into core-api Joe Perches 1 sibling, 2 replies; 16+ messages in thread From: Joe Perches @ 2019-07-23 13:51 UTC (permalink / raw) To: Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes, Andrew Morton 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-07-23 13:51 ` [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms Joe Perches @ 2019-07-23 14:37 ` Rasmus Villemoes 2019-07-23 15:39 ` Joe Perches 2019-09-25 21:50 ` Andrew Morton 1 sibling, 1 reply; 16+ messages in thread From: Rasmus Villemoes @ 2019-07-23 14:37 UTC (permalink / raw) To: Joe Perches, Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Andrew Morton 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-07-23 14:37 ` Rasmus Villemoes @ 2019-07-23 15:39 ` Joe Perches 2019-07-24 6:53 ` Rasmus Villemoes 0 siblings, 1 reply; 16+ messages in thread From: Joe Perches @ 2019-07-23 15:39 UTC (permalink / raw) To: Rasmus Villemoes, Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Andrew Morton 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-07-23 15:39 ` Joe Perches @ 2019-07-24 6:53 ` Rasmus Villemoes 2019-07-24 7:10 ` Joe Perches 0 siblings, 1 reply; 16+ messages in thread From: Rasmus Villemoes @ 2019-07-24 6:53 UTC (permalink / raw) To: Joe Perches, Rasmus Villemoes, Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Andrew Morton 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-07-24 6:53 ` Rasmus Villemoes @ 2019-07-24 7:10 ` Joe Perches 0 siblings, 0 replies; 16+ messages in thread From: Joe Perches @ 2019-07-24 7:10 UTC (permalink / raw) To: Rasmus Villemoes, Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Andrew Morton 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); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-07-23 13:51 ` [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms Joe Perches 2019-07-23 14:37 ` Rasmus Villemoes @ 2019-09-25 21:50 ` Andrew Morton 2019-09-26 0:01 ` Stephen Kitt 2019-09-26 8:34 ` Joe Perches 1 sibling, 2 replies; 16+ messages in thread From: Andrew Morton @ 2019-09-25 21:50 UTC (permalink / raw) To: Joe Perches Cc: Linus Torvalds, linux-kernel, Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-25 21:50 ` Andrew Morton @ 2019-09-26 0:01 ` Stephen Kitt 2019-09-26 7:29 ` Rasmus Villemoes 2019-09-26 8:34 ` Joe Perches 1 sibling, 1 reply; 16+ messages in thread From: Stephen Kitt @ 2019-09-26 0:01 UTC (permalink / raw) To: Andrew Morton Cc: Joe Perches, Linus Torvalds, linux-kernel, Jonathan Corbet, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes, Takashi Iwai, Clemens Ladisch, alsa-devel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-26 0:01 ` Stephen Kitt @ 2019-09-26 7:29 ` Rasmus Villemoes 2019-09-26 8:25 ` Stephen Kitt 0 siblings, 1 reply; 16+ messages in thread From: Rasmus Villemoes @ 2019-09-26 7:29 UTC (permalink / raw) To: Stephen Kitt, Andrew Morton Cc: Joe Perches, Linus Torvalds, linux-kernel, Jonathan Corbet, Kees Cook, Nitin Gote, jannh, kernel-hardening, Takashi Iwai, Clemens Ladisch, alsa-devel 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"); \ ^ ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-26 7:29 ` Rasmus Villemoes @ 2019-09-26 8:25 ` Stephen Kitt 2019-09-26 8:51 ` Rasmus Villemoes 0 siblings, 1 reply; 16+ messages in thread From: Stephen Kitt @ 2019-09-26 8:25 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Joe Perches, Linus Torvalds, linux-kernel, Jonathan Corbet, Kees Cook, Nitin Gote, jannh, kernel-hardening, Takashi Iwai, Clemens Ladisch, alsa-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-26 8:25 ` Stephen Kitt @ 2019-09-26 8:51 ` Rasmus Villemoes 0 siblings, 0 replies; 16+ messages in thread From: Rasmus Villemoes @ 2019-09-26 8:51 UTC (permalink / raw) To: Stephen Kitt Cc: Andrew Morton, Joe Perches, Linus Torvalds, linux-kernel, Jonathan Corbet, Kees Cook, Nitin Gote, jannh, kernel-hardening, Takashi Iwai, Clemens Ladisch, alsa-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-25 21:50 ` Andrew Morton 2019-09-26 0:01 ` Stephen Kitt @ 2019-09-26 8:34 ` Joe Perches 2019-09-26 15:45 ` Kees Cook ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Joe Perches @ 2019-09-26 8:34 UTC (permalink / raw) To: Andrew Morton, Julia Lawall Cc: Linus Torvalds, linux-kernel, Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes 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"); ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-26 8:34 ` Joe Perches @ 2019-09-26 15:45 ` Kees Cook 2019-09-27 12:57 ` Julia Lawall 2019-09-27 13:22 ` Julia Lawall 2 siblings, 0 replies; 16+ messages in thread From: Kees Cook @ 2019-09-26 15:45 UTC (permalink / raw) To: Joe Perches Cc: Andrew Morton, Julia Lawall, Linus Torvalds, linux-kernel, Jonathan Corbet, Stephen Kitt, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-26 8:34 ` Joe Perches 2019-09-26 15:45 ` Kees Cook @ 2019-09-27 12:57 ` Julia Lawall 2019-09-27 13:22 ` Julia Lawall 2 siblings, 0 replies; 16+ messages in thread From: Julia Lawall @ 2019-09-27 12:57 UTC (permalink / raw) To: Joe Perches Cc: Andrew Morton, Linus Torvalds, linux-kernel, Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes 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"); > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms 2019-09-26 8:34 ` Joe Perches 2019-09-26 15:45 ` Kees Cook 2019-09-27 12:57 ` Julia Lawall @ 2019-09-27 13:22 ` Julia Lawall 2 siblings, 0 replies; 16+ messages in thread From: Julia Lawall @ 2019-09-27 13:22 UTC (permalink / raw) To: Joe Perches Cc: Andrew Morton, Linus Torvalds, linux-kernel, Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes 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"); > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V2 2/2] kernel-doc: core-api: Include string.h into core-api 2019-07-23 13:51 [PATCH V2 0/2] string: Add stracpy and stracpy_pad Joe Perches 2019-07-23 13:51 ` [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms Joe Perches @ 2019-07-23 13:51 ` Joe Perches 1 sibling, 0 replies; 16+ messages in thread From: Joe Perches @ 2019-07-23 13:51 UTC (permalink / raw) To: Linus Torvalds, linux-kernel Cc: Jonathan Corbet, Stephen Kitt, Kees Cook, Nitin Gote, jannh, kernel-hardening, Rasmus Villemoes, Andrew Morton, linux-doc 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-09-27 13:22 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-23 13:51 [PATCH V2 0/2] string: Add stracpy and stracpy_pad Joe Perches 2019-07-23 13:51 ` [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms Joe Perches 2019-07-23 14:37 ` Rasmus Villemoes 2019-07-23 15:39 ` Joe Perches 2019-07-24 6:53 ` Rasmus Villemoes 2019-07-24 7:10 ` Joe Perches 2019-09-25 21:50 ` Andrew Morton 2019-09-26 0:01 ` Stephen Kitt 2019-09-26 7:29 ` Rasmus Villemoes 2019-09-26 8:25 ` Stephen Kitt 2019-09-26 8:51 ` Rasmus Villemoes 2019-09-26 8:34 ` Joe Perches 2019-09-26 15:45 ` Kees Cook 2019-09-27 12:57 ` Julia Lawall 2019-09-27 13:22 ` Julia Lawall 2019-07-23 13:51 ` [PATCH V2 2/2] kernel-doc: core-api: Include string.h into core-api Joe Perches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).