linux-kernel - Re: [PATCH] iio: imu: st_lsm6dsx: check st_lsm6dsx_shub_read

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: <CAHp75Vcomp1mDUm5houAm2j2b2HtxAP8Z78Vfcdmgm_g+bbt7w@mail.gmail.com>
Date:   Mon, 10 Aug 2020 11:08:39 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     trix@...hat.com
Cc:     Lorenzo Bianconi <lorenzo.bianconi83@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald <pmeerw@...erw.net>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: imu: st_lsm6dsx: check st_lsm6dsx_shub_read_output return

On Sun, Aug 9, 2020 at 8:56 PM <trix@...hat.com> wrote:
>
> From: Tom Rix <trix@...hat.com>
>
> clang static analysis reports this represenative problem
>
> st_lsm6dsx_shub.c:540:8: warning: Assigned value is garbage or undefined
>         *val = (s16)le16_to_cpu(*((__le16 *)data));
>              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> data is set with
>
>         err = st_lsm6dsx_shub_read(sensor, ch->address, data, len);
>         if (err < 0)
>                 return err;
>
> The problem with st_lsm6dsx_shub_read() is this statement
>
>         err = st_lsm6dsx_shub_read_output(hw, data,
>                                           len & ST_LS6DSX_READ_OP_MASK);
>
> The err value is never checked.
> So check err.
>


> Fixes: c91c1c844ebd ("iio: imu: st_lsm6dsx: add i2c embedded controller support")
>
> Signed-off-by: Tom Rix <trix@...hat.com>

You see, the commit message can be divided to three sections

1. Title / very short description
2. Detailed description
3. Tag block

Each of them has some specific rules:
1. One quite short line prefixed by subsystem / driver in the
specified format (usually gathered by reading git log against the
module in question)
2. Should explain why this change is done
3. Should be one tag -- one line, no blank lines in between.

Hope, you will use this in the future.

After addressing that (perhaps Jonathan will do it for you)
Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>

> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> index ed83471dc7dd..8c8d8870ca07 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c
> @@ -313,6 +313,8 @@ st_lsm6dsx_shub_read(struct st_lsm6dsx_sensor *sensor, u8 addr,
>
>         err = st_lsm6dsx_shub_read_output(hw, data,
>                                           len & ST_LS6DSX_READ_OP_MASK);
> +       if (err < 0)
> +               return err;
>
>         st_lsm6dsx_shub_master_enable(sensor, false);
>
> --
> 2.18.1
>


-- 
With Best Regards,
Andy Shevchenko