linux-kernel - Re: [PATCH 3/8] powerpc/nvram: Move an assignment for the variable "ret" in dev_nvram

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <52f46c9b-e049-7288-2c53-9c2525cd84c6@linux.vnet.ibm.com>
Date:   Thu, 19 Jan 2017 16:24:17 -0800
From:   Tyrel Datwyler <tyreld@...ux.vnet.ibm.com>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>,
        linuxppc-dev@...ts.ozlabs.org,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Daniel Axtens <dja@...ens.net>,
        Geliang Tang <geliangtang@....com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Nathan Fontenot <nfont@...ux.vnet.ibm.com>,
        Pan Xinhui <xinhui.pan@...ux.vnet.ibm.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Paul Mackerras <paulus@...ba.org>
Cc:     kernel-janitors@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/8] powerpc/nvram: Move an assignment for the variable
 "ret" in dev_nvram_write()

On 01/19/2017 08:56 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Thu, 19 Jan 2017 15:55:36 +0100
> 
> A local variable was set to an error code before a concrete error situation
> was detected. Thus move the corresponding assignment into an if branch
> to indicate a software failure there.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  arch/powerpc/kernel/nvram_64.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
> index cf839adf3aa7..dc90a0e9ad65 100644
> --- a/arch/powerpc/kernel/nvram_64.c
> +++ b/arch/powerpc/kernel/nvram_64.c
> @@ -806,9 +806,10 @@ static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
>  	if (!tmp)
>  		return -ENOMEM;
> 
> -	ret = -EFAULT;
> -	if (copy_from_user(tmp, buf, count))
> +	if (copy_from_user(tmp, buf, count)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>
>  	ret = ppc_md.nvram_write(tmp, count, ppos);
> 

I think you really could have squashed patches 1-3 into a single patch
that returns directly after any failure. After this 3rd patch this is
now the only spot that branches to the "out" label. At this point you
might as well remove that label and move the kfree(tmp) call up and
return directly after the failure and at the nvram_write() call site
doing away completely with the "ret" variable.

Something like the following patch:

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index d5e2b83..eadb55c 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -789,37 +789,29 @@ static ssize_t dev_nvram_read(struct file *file,
char __user *buf,
 static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
                          size_t count, loff_t *ppos)
 {
-       ssize_t ret;
-       char *tmp = NULL;
+       char *tmp;
        ssize_t size;

-       ret = -ENODEV;
        if (!ppc_md.nvram_size)
-               goto out;
+               return -ENODEV;

-       ret = 0;
        size = ppc_md.nvram_size();
        if (*ppos >= size || size < 0)
-               goto out;
+               return 0;

        count = min_t(size_t, count, size - *ppos);
        count = min(count, PAGE_SIZE);

-       ret = -ENOMEM;
        tmp = kmalloc(count, GFP_KERNEL);
        if (!tmp)
-               goto out;
-
-       ret = -EFAULT;
-       if (copy_from_user(tmp, buf, count))
-               goto out;
-
-       ret = ppc_md.nvram_write(tmp, count, ppos);
+               return -ENOMEM;

-out:
-       kfree(tmp);
-       return ret;
+       if (copy_from_user(tmp, buf, count)) {
+               kfree(tmp);
+               return -EFAULT;
+       }

+       return ppc_md.nvram_write(tmp, count, ppos);
 }

 static long dev_nvram_ioctl(struct file *file, unsigned int cmd,