linux-kernel - Re: [PATCH 2/2] i2c: xiic: Add atomic transfer support

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: <nseg7dpgansb6lpxj4co37duafzwiyt4spgolltuir47dedcsc@eyvam2o4o623>
Date: Mon, 25 Nov 2024 20:22:52 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Kees Bakker <kees@...erbout.nl>
Cc: Manikanta Guntupalli <manikanta.guntupalli@....com>, git@....com, 
	michal.simek@....com, linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org, radhey.shyam.pandey@....com, srinivas.goud@....com, 
	shubhrajyoti.datta@....com, manion05gk@...il.com
Subject: Re: [PATCH 2/2] i2c: xiic: Add atomic transfer support

Hi Kees,

On Fri, Nov 22, 2024 at 10:29:27PM +0100, Kees Bakker wrote:
> Op 11-10-2024 om 12:41 schreef Manikanta Guntupalli:
> > Rework the read and write code paths in the driver to support operation
> > in atomic contexts.
> > 
> > Similar changes have been implemented in other drivers, including:
> > commit 3a5ee18d2a32 ("i2c: imx: implement master_xfer_atomic callback")
> > commit 445094c8a9fb ("i2c: exynos5: add support for atomic transfers")
> > commit ede2299f7101 ("i2c: tegra: Support atomic transfers")
> > commit fe402bd09049 ("i2c: meson: implement the master_xfer_atomic
> > callback")
> > 
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@....com>
> > ---
> >   drivers/i2c/busses/i2c-xiic.c | 245 +++++++++++++++++++++++++++++-----
> >   1 file changed, 212 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> > index 052bae4fc664..e5e9a4993bd4 100644
> > --- a/drivers/i2c/busses/i2c-xiic.c
> > +++ b/drivers/i2c/busses/i2c-xiic.c
> > [...]
> > +static void xiic_recv_atomic(struct xiic_i2c *i2c)
> > +{
> > +	while (xiic_rx_space(i2c)) {
> Let's remind what xiic_rx_space is
> #define xiic_rx_space(i2c) ((i2c)->rx_msg->len - (i2c)->rx_pos)
> 
> > +		if (xiic_getreg32(i2c, XIIC_IISR_OFFSET) & XIIC_INTR_RX_FULL_MASK) {
> > +			if (!i2c->rx_msg) {
> This check is suspicious. If i2c->rx_msg is NULL then the while
> above already dereferenced a NULL pointer.
> What is going on?
> > [...]
> > +}
> > [...]
> > +static void xiic_send_rem_atomic(struct xiic_i2c *i2c)
> > +{
> > +	if (!i2c->tx_msg)
> > +		goto send_atomic_out;
> > +	while (xiic_tx_space(i2c)) {
> > [...]
> > +	}
> > +send_atomic_out:
> > +	if (i2c->nmsgs > 1) {
> > +		i2c->nmsgs--;
> > +		i2c->tx_msg++;
> We can get here with i2c->tx_msg being NULL. See the first if
> in the function.

Valid point. Thanks for looking into this and thanks Wolfram for
having removed it. I read this patch several times, but I missed
it.

Andi