On 10/20/2014 04:40 PM, Takashi Iwai wrote:
At Sun, 19 Oct 2014 11:38:58 +0200, Takashi Iwai wrote:
At Sun, 19 Oct 2014 09:11:26 +0200, Daniel Mack wrote:
Out of principles, use strncpy() in favor of strcpy(). That is, however, an insignificant detail here.
Signed-off-by: Daniel Mack daniel@zonque.org
Well, blindly doing this isn't optimal, IMO. First off, strlcpy() is a better one. And, in the code you patched, we already know all strings to be passed. That is, if anything is over the buffer size, it's a clear bug. This can be caught by static analyzers, or put some debug codes (either for build time or compile time) instead of silently trimming the string.
BTW, there is already a nice helper function, snd_ctl_enum_info(), for the safe enum info setup. Then the patch would become even more reducing, something like below. We can cover many other places in similar ways.
A further step would be to add a kernel warning when the given string is too long as an enum item string. Then we can catch the buggy driver, too. I'll cook up the patch.
Sounds good to me. And yes, we know which values we write to that buffer and that they can't possibly explode it. I was just thinking of the next reader of the code who might be quicker in the review with some obvious guards in place.
Thanks, Daniel
Takashi
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index f119a41ed9a9..dd4d5bdea423 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -441,15 +441,7 @@ static int snd_emu0204_ch_switch_info(struct snd_kcontrol *kcontrol, "3/4" };
- uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
- uinfo->count = 1;
- uinfo->value.enumerated.items = 2;
- if (uinfo->value.enumerated.item > 1)
uinfo->value.enumerated.item = 1;- strcpy(uinfo->value.enumerated.name,
texts[uinfo->value.enumerated.item]);- return 0;
- return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
}
static int snd_emu0204_ch_switch_get(struct snd_kcontrol *kcontrol, @@ -745,15 +737,7 @@ static int snd_ftu_eff_switch_info(struct snd_kcontrol *kcontrol, "Echo" };
- uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
- uinfo->count = 1;
- uinfo->value.enumerated.items = 8;
- if (uinfo->value.enumerated.item > 7)
uinfo->value.enumerated.item = 7;- strcpy(uinfo->value.enumerated.name,
texts[uinfo->value.enumerated.item]);- return 0;
- return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts);
}
static int snd_ftu_eff_switch_get(struct snd_kcontrol *kctl,