linux-kernel - Re: [PATCH v2] i2c: xgene-slimpro: Fix out-of-bounds bug in xgene_slimpro_i2c

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

Message-ID: <20230314155759.ej2gax7r4ek7itmh@intel.intel>
Date:   Tue, 14 Mar 2023 16:57:59 +0100
From:   Andi Shyti <andi.shyti@...nel.org>
To:     Wei Chen <harperchen1110@...il.com>
Cc:     Andi Shyti <andi.shyti@...nel.org>, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] i2c: xgene-slimpro: Fix out-of-bounds bug in
 xgene_slimpro_i2c_xfer()

Hi Wei,

On Tue, Mar 14, 2023 at 11:43:41PM +0800, Wei Chen wrote:
> The data->block[0] variable comes from user and is a number between
> 0-255. Without a proper check, the variable may be very large to cause
> an out-of-bounds when performing memcpy in slimpro_i2c_blkwr.
> 
> Fix this bug by checking the value of writelen.
> 
> Signed-off-by: Wei Chen <harperchen1110@...il.com>

I forgot to check earlier, can you also add:

Fixes: f6505fbabc42 ("i2c: add SLIMpro I2C device driver on APM X-Gene platform")
Cc: stable@...r.kernel.org

> ---
> Changes in v2:
>  - Put length check inside slimpro_i2c_blkwr
> 
> drivers/i2c/busses/i2c-xgene-slimpro.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c
> b/drivers/i2c/busses/i2c-xgene-slimpro.c
> index bc9a3e7e0c96..0f7263e2276a 100644
> --- a/drivers/i2c/busses/i2c-xgene-slimpro.c
> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
> @@ -308,6 +308,9 @@ static int slimpro_i2c_blkwr(struct
> slimpro_i2c_dev *ctx, u32 chip,
> u32 msg[3];
> int rc;
> + if (writelen > I2C_SMBUS_BLOCK_MAX)
> + return -EINVAL;
> +

There is something odd looking here. Can you please fix the
formatting and leave one blank line from the variable declaration
and the 'if (...'.

Remember, please, to run checkpatch.pl before sending the patch.

Andi