linux-kernel - Re: [PATCH] ALSA: emu10k1: Refactor copy_to_user() usage in snd_emu10k1_fx8010

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <d4dfd310-cf03-4b51-914f-34c46334005b@web.de>
Date: Mon, 16 Sep 2024 09:33:16 +0200
From: Markus Elfring <Markus.Elfring@....de>
To: Takashi Iwai <tiwai@...e.de>, linux-sound@...r.kernel.org
Cc: Takashi Iwai <tiwai@...e.com>, LKML <linux-kernel@...r.kernel.org>,
 Jaroslav Kysela <perex@...ex.cz>, Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [PATCH] ALSA: emu10k1: Refactor copy_to_user() usage in
 snd_emu10k1_fx8010_ioctl()

>> From: Markus Elfring <elfring@...rs.sourceforge.net>
>> Date: Sun, 15 Sep 2024 16:16:45 +0200
>>
>> Assign values from selected expressions where copy_to_user() calls
>> are involved to additional local variables so that the number of
>> kfree() calls can be reduced accordingly.
>>
>> This issue was transformed by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
>
> This doesn't look like an improvement.  With the refactoring, it's
> rather harder to read.
…
>> ---
>>  sound/pci/emu10k1/emufx.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
>> index 03efc317e05f..2f20878a78c9 100644
>> --- a/sound/pci/emu10k1/emufx.c
>> +++ b/sound/pci/emu10k1/emufx.c
>> @@ -2496,12 +2496,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>>  		if (!info)
>>  			return -ENOMEM;
>>  		snd_emu10k1_fx8010_info(emu, info);
>> -		if (copy_to_user(argp, info, sizeof(*info))) {
>> +
>> +		{
>> +			unsigned long ctu = copy_to_user(argp, info, sizeof(*info));
>>  			kfree(info);
>> -			return -EFAULT;
>> +			return ctu ? -EFAULT : 0;
>>  		}
>> -		kfree(info);
>> -		return 0;
>>  	case SNDRV_EMU10K1_IOCTL_CODE_POKE:
>>  		if (!capable(CAP_SYS_ADMIN))
>>  			return -EPERM;
>> @@ -2517,12 +2517,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>>  		if (IS_ERR(icode))
>>  			return PTR_ERR(icode);
>>  		res = snd_emu10k1_icode_peek(emu, icode);
>> -		if (res == 0 && copy_to_user(argp, icode, sizeof(*icode))) {
>> +
>> +		{
>> +			bool ctu = res == 0 && copy_to_user(argp, icode, sizeof(*icode));
>>  			kfree(icode);
>> -			return -EFAULT;
>> +			return ctu ? -EFAULT : res;
>>  		}
>> -		kfree(icode);
>> -		return res;
>>  	case SNDRV_EMU10K1_IOCTL_PCM_POKE:
>>  		ipcm = memdup_user(argp, sizeof(*ipcm));
>>  		if (IS_ERR(ipcm))
>> @@ -2535,12 +2535,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>>  		if (IS_ERR(ipcm))
>>  			return PTR_ERR(ipcm);
>>  		res = snd_emu10k1_ipcm_peek(emu, ipcm);
>> -		if (res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm))) {
>> +
>> +		{
>> +			bool ctu = res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm));
>>  			kfree(ipcm);
>> -			return -EFAULT;
>> +			return ctu ? -EFAULT : res;
>>  		}
>> -		kfree(ipcm);
>> -		return res;
>>  	case SNDRV_EMU10K1_IOCTL_TRAM_SETUP:
>>  		if (!capable(CAP_SYS_ADMIN))
>>  			return -EPERM;
>> --
>> 2.46.0
>>

* Can three less kfree() calls matter here finally?

* Do you find the affected control flow really “too succinct” now?

Regards,
Markus