ALSA: control-led: use strscpy() to instead of strncpy()

PATCH In review [email protected] 4 participants 4 messages Jan 9, 2023 Sound

lore

Share & tools

lore

[email protected]

author

2023-01-09 11:46 UTC #0 lore raw

PATCH 1/1 ALSA: control-led: use strscpy() to instead of strncpy()

From: Xu Panda <[email protected]>

The implementation of strscpy() is more robust and safer. That's now the recommended way to copy NUL-terminated strings.

Signed-off-by Xu Panda <[email protected]>

Signed-off-by Yang Yang <[email protected]>

---
 sound/core/control_led.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

sound/core/control_led.c +1 -2

index f975cc85772b..c88653c205eb 100644
@@ -534,8 +534,7 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
 	struct snd_ctl_elem_id id;
 	int err;
 
-	strncpy(buf2, buf, len);
-	buf2[len] = '\0';
+	strncpy(buf2, buf, len + 1);
 	memset(&id, 0, sizeof(id));
 	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	s = buf2;

Signature · 1 line

2.15.2

Takashi Iwai

2023-01-09 12:40 UTC #1 lore raw

Quoted 17 lines · On Mon, 09 Jan 2023 12:45:51 +0100, <[email protected]> wrote:

From: Xu Panda <[email protected]>
The implementation of strscpy() is more robust and safer. That's now the recommended way to copy NUL-terminated strings.
Signed-off-by Xu Panda <[email protected]>
Signed-off-by Yang Yang <[email protected]>
---
 sound/core/control_led.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
sound/core/control_led.c +0 -0
index f975cc85772b..c88653c205eb 100644

sound/core/control_led.c static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si · line 537

Quoted 6 lines

sound/core/control_led.c +1 -2

@@ -534,8 +534,7 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
 	struct snd_ctl_elem_id id;
 	int err;
 
-	strncpy(buf2, buf, len);
-	buf2[len] = '\0';
+	strncpy(buf2, buf, len + 1);

Still using strncpy()...?

thanks,

Takashi

Jaroslav Kysela

maintainer

2023-01-09 13:37 UTC #2 lore raw

Quoted 22 lines · Takashi Iwai

Quoted 20 lines · On Mon, 09 Jan 2023 12:45:51 +0100, <[email protected]> wrote:
From: Xu Panda <[email protected]>
The implementation of strscpy() is more robust and safer. That's now the recommended way to copy NUL-terminated strings.
Signed-off-by Xu Panda <[email protected]>
Signed-off-by Yang Yang <[email protected]>
---
  sound/core/control_led.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)
sound/core/control_led.c +0 -0
index f975cc85772b..c88653c205eb 100644
@@ -534,8 +534,7 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
  	struct snd_ctl_elem_id id;
  	int err;
 

Quoted 5 lines

sound/core/control_led.c static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si · line 537
Quoted 3 lines
sound/core/control_led.c +1 -2
-	strncpy(buf2, buf, len);
-	buf2[len] = '\0';
+	strncpy(buf2, buf, len + 1);
Still using strncpy()...?

The original code should be:

sound/core/control_led.c +1 -1

index f975cc85772b..b44abefcb593 100644
@@ -530,7 +530,7 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
                          bool attach)
 {
        char buf2[256], *s, *os;
-       size_t len = max(sizeof(s) - 1, count);
+       size_t len = min(sizeof(buf2) - 1, count);
        struct snd_ctl_elem_id id;
        int err;

But it can be replaced with:

sound/core/control_led.c +2 -3

index f975cc85772b..41d4e898d34c 100644
@@ -530,12 +530,11 @@ static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, si
                          bool attach)
 {
        char buf2[256], *s, *os;
-       size_t len = max(sizeof(s) - 1, count);
        struct snd_ctl_elem_id id;
        int err;
 
-       strncpy(buf2, buf, len);
-       buf2[len] = '\0';
+       if (strscpy(buf2, buf, count) < 0)
+               return -E2BIG;
        memset(&id, 0, sizeof(id));
        id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
        s = buf2;

I'll send a patch ASAP.

Jaroslav

--
Jaroslav Kysela <[email protected]>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Takashi Sakamoto

2023-01-09 12:58 UTC #3 lore raw

In reply to [email protected]

Hi,

Quoted 16 lines · On Mon, Jan 9, 2023, at 20:45, [email protected] wrote:

From: Xu Panda <[email protected]>
The implementation of strscpy() is more robust and safer. That's now the recommended way to copy NUL-terminated strings.
Signed-off-by Xu Panda <[email protected]>
Signed-off-by Yang Yang <[email protected]>
---
 sound/core/control_led.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
sound/core/control_led.c +0 -0
index f975cc85772b..c88653c205eb 100644

sound/core/control_led.c static ssize_t set_led_id(struct snd_ctl_led_card · line 538

Quoted 7 lines

sound/core/control_led.c +1 -2

@@ -534,8 +534,7 @@ static ssize_t set_led_id(struct snd_ctl_led_card 
 *led_card, const char *buf, si
 	struct snd_ctl_elem_id id;
 	int err;
 
-	strncpy(buf2, buf, len);
-	buf2[len] = '\0';
+	strncpy(buf2, buf, len + 1);

The patch comment refers to strscpy(), however strncpy() is still used. I wonder whether it is the intension of this patch. I think any trouble happended.

Anyway I'm for usage of strscpy() as the comment.

Quoted 5 lines

sound/core/control_led.c +0 -0
 	memset(&id, 0, sizeof(id));
 	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
s = buf2;
Signature · 1 line
2.15.2

As another issue, I can see that the local variable, len, can bring buffer overrun over buf2[256] since it has maximum value between the size of pointer and count argument. Maricious user space application can attack as long as it has write permission to the device attributes. I guess kernel stack can be broken by the attack.

```
532    char buf2[256], *s, *os;
533    size_t len = max(sizeof(s) - 1, count);
...
537    strncpy(buf2, buf, len);
```

I'm already in bed today, so I hope anyone posts fix, or waiting tomorrow.

Regards

Takashi Sakamoto