linux-kernel - Re: [PATCH] nvmem: properly handle returned value nvmem_reg

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]

Date:   Sun, 6 May 2018 16:58:08 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Mathieu Malaterre <malat@...ian.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvmem: properly handle returned value nvmem_reg_read

On Sat, May 5, 2018 at 11:24 PM, Mathieu Malaterre <malat@...ian.org> wrote:
> Function nvmem_reg_read can return a non zero value indicating an error.
> This returned value must be read and error propagated to
> nvmem_cell_prepare_write_buffer.

Must is to strong word here. How come it *must*?
Did you investigate the history of the changes that brought us to current code?

How had you tested your change?
While it looks subtle it is in one of the storage class devices core,
which might be very sensible to changing workflow.

> Silence the following gcc warning (W=1):
>
>   drivers/nvmem/core.c:1093:9: warning: variable ‘rc’ set but not used [-Wunused-but-set-variable]

This is least argument to accept this patch.

>
> Signed-off-by: Mathieu Malaterre <malat@...ian.org>
> ---
>  drivers/nvmem/core.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b05aa8e81303..f34f2363925a 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1107,6 +1107,8 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
>
>                 /* setup the first byte with lsb bits from nvmem */
>                 rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
> +               if (rc)
> +                       goto err;
>                 *b++ |= GENMASK(bit_offset - 1, 0) & v;
>
>                 /* setup rest of the byte if any */
> @@ -1125,11 +1127,16 @@ static void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
>                 /* setup the last byte with msb bits from nvmem */
>                 rc = nvmem_reg_read(nvmem,
>                                     cell->offset + cell->bytes - 1, &v, 1);
> +               if (rc)
> +                       goto err;
>                 *p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
>
>         }
>
>         return buf;
> +err:
> +       kfree(buf);
> +       return NULL;
>  }
>
>  /**
> --
> 2.11.0
>



-- 
With Best Regards,
Andy Shevchenko