Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <81c898ca-cf43-d6b8-8686-a274c7adf3be@xs4all.nl>
Date:   Wed, 19 Jul 2023 09:34:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] Initialization of read buffer for dib3000_read_reg
Content-Language: en-US
To:     Kernel-Development <kdev@benbenng.net>,
        "mchehab@kernel.org" <mchehab@kernel.org>
Cc:     "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "skhan@linuxfoundation.org" <skhan@linuxfoundation.org>,
        "linux-kernel-mentees@lists.linuxfoundation.org" 
        <linux-kernel-mentees@lists.linuxfoundation.org>,
        "syzbot+c88fc0ebe0d5935c70da@syzkaller.appspotmail.com" 
        <syzbot+c88fc0ebe0d5935c70da@syzkaller.appspotmail.com>
References: <20230413091841.22000-1-kdev@benbenng.net>
 <0100018779eb40dc-cee9e39d-5d87-4733-83db-eca5218fcc8f-000000@email.amazonses.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
In-Reply-To: <0100018779eb40dc-cee9e39d-5d87-4733-83db-eca5218fcc8f-000000@email.amazonses.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

Some comments on this patch:

On 13/04/2023 11:21, Kernel-Development wrote:
> This is a patch that fixes a bug:
> KMSAN: uninit-value in dib3000mb_attach (2)
> 
> Local variable u8 rb[2] is not initialized as it is used as read buffer
> for i2c_transfer(). It is expected that i2c_transfer() should fill in
> the buffer before the target function returns rb's content. However
> error handling of i2c_transfer is not done, and on occasions where the
> read fails, uninitialized rb value will be returned.
> 
> The usage of this function, defined as macro rd() in
> drivers/media/dvb-frontends/dib3000mb_priv,h, does not expect any error
> to occur. Adding error handling here might involve significant code
> changes.
> 
> Thus 0-initialization is done on rb. This might affect some logic on
> error case as the use of the return value is used as boolean and flags.
> 
> Reported-by: syzbot+c88fc0ebe0d5935c70da@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=2f4d19de8c9e9f0b9794e53ca54d68e0ffe9f068
> Signed-off-by: (Ben) HokChun Ng <kdev@benbenng.net>
> ---
>  drivers/media/dvb-frontends/dib3000mb.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/dib3000mb.c b/drivers/media/dvb-frontends/dib3000mb.c
> index a6c2fc4586eb..0dd96656aaf4 100644
> --- a/drivers/media/dvb-frontends/dib3000mb.c
> +++ b/drivers/media/dvb-frontends/dib3000mb.c
> @@ -50,15 +50,19 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,2=xfer,4=setfe,8=getfe (|-a
>  
>  static int dib3000_read_reg(struct dib3000_state *state, u16 reg)
>  {
> +	int errno;
>  	u8 wb[] = { ((reg >> 8) | 0x80) & 0xff, reg & 0xff };
> -	u8 rb[2];
> +	u8 rb[2] = { 0, 0 };

Really all you need to do here is zero this array, which can be even
shorter by writing: u8 rb[2] = {};

It is enough to just show the "i2c read error" message, nothing else
is needed here.

BTW, checkpatch.pl also complains about your email address ('Kernel-Development <kdev@benbenng.net>'
being different from your SoB line: (Ben) HokChun Ng <kdev@benbenng.net>.

It's a good idea to ensure the two are the same. I would stick to
(Ben) HokChun Ng <kdev@benbenng.net> since that has your actual name.

Regards,

	Hans

>  	struct i2c_msg msg[] = {
>  		{ .addr = state->config.demod_address, .flags = 0,        .buf = wb, .len = 2 },
>  		{ .addr = state->config.demod_address, .flags = I2C_M_RD, .buf = rb, .len = 2 },
>  	};
>  
> -	if (i2c_transfer(state->i2c, msg, 2) != 2)
> -		deb_i2c("i2c read error\n");
> +	errno = i2c_transfer(state->i2c, msg, 2);
> +	if (errno != 2) {
> +		deb_i2c("i2c read error (errno: %d)\n", -errno);
> +		return 0;
> +	}
>  
>  	deb_i2c("reading i2c bus (reg: %5d 0x%04x, val: %5d 0x%04x)\n",reg,reg,
>  			(rb[0] << 8) | rb[1],(rb[0] << 8) | rb[1]);