Re: [PATCH v4 1/5] mfd: Add support for Intel LJCA device

From: "Ye, Xiang" <xiang.ye@intel.com>
To: Oliver Neukum <oneukum@suse.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Matthias Kaehlcke <mka@chromium.org>, Lee Jones <lee@kernel.org>,
	Wolfram Sang <wsa@kernel.org>, Tyrone Ting <kfting@nuvoton.com>,
	Mark Brown <broonie@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>, <linux-usb@vger.kernel.org>,
	<linux-i2c@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-spi@vger.kernel.org>, <linux-gpio@vger.kernel.org>,
	<srinivas.pandruvada@intel.com>,
	<heikki.krogerus@linux.intel.com>,
	<andriy.shevchenko@linux.intel.com>,
	<sakari.ailus@linux.intel.com>, <zhifeng.wang@intel.com>,
	<wentong.wu@intel.com>, <lixu.zhang@intel.com>
Subject: Re: [PATCH v4 1/5] mfd: Add support for Intel LJCA device
Date: Tue, 14 Mar 2023 15:15:05 +0800	[thread overview]
Message-ID: <ZBAe+bpKmvC1iVfq@ye-NUC7i7DNHE> (raw)
In-Reply-To: <ca995ed7-e6db-4265-e528-5e29fb418594@suse.com>

Hi Oliver,

Thanks for the review.
On Mon, Mar 13, 2023 at 02:27:50PM +0100, Oliver Neukum wrote:
> On 10.03.23 05:14, Ye, Xiang wrote:
> > Hi Oliver,
> 
> Hi,
> 
> sorry for the delayed answer.
No problem.
> 
> > Thanks for your review.
> > On Thu, Mar 09, 2023 at 01:53:28PM +0100, Oliver Neukum wrote:
> > > 
> > > 
> > > On 09.03.23 08:10, Ye Xiang wrote:
> > > 
> > > > +static int ljca_stub_write(struct ljca_stub *stub, u8 cmd, const void *obuf, unsigned int obuf_len,
> > > > +			   void *ibuf, unsigned int *ibuf_len, bool wait_ack, unsigned long timeout)
> > > 
> > > Why do you make ibuf_len a pointer?
> > Because ibuf_len is also used as output of this function here.
> > It stores the actual length of ibuf receive from LJCA device.
> 
> Yes, I understand that now, thank you for the explanation, yet
> that is problematic, if we look at another issue. See further down:
> 
> > > > +		ret = -ENODEV;
> > > > +		goto error_put;
> > > > +	}
> > > > +
> > > > +	mutex_lock(&dev->mutex);
> > > > +	stub->cur_cmd = cmd;
> > > > +	stub->ipacket.ibuf = ibuf;
> > > > +	stub->ipacket.ibuf_len = ibuf_len;
> 
> Here you store the pointer into the stub. Hence we must make sure
> that the location it points to stays valid.
> 

> Now let's look at ljca_mng_reset_handshake(). I am afraid I have to quote
> its first part in full:
> 
> +static int ljca_mng_reset_handshake(struct ljca_stub *stub)
> +{
> +	struct ljca_mng_priv *priv;
> +	__le32 reset_id;
> +	__le32 reset_id_ret = 0;
> +	unsigned int ilen = sizeof(__le32);
> 
> This is on the _stack_
> Highly important !!!
> 
> +	int ret;
> +
> +	priv = ljca_priv(stub);
> +	reset_id = cpu_to_le32(priv->reset_id++);
> +	ret = ljca_stub_write(stub, LJCA_MNG_RESET_NOTIFY, &reset_id, sizeof(reset_id),
> +			      &reset_id_ret, &ilen, true, LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> 
> If we run into the timeout error case, ret will be -ETIMEDOUT.
> 
> +	if (ret)
> +		return ret;
> 
> And thus here we return and free the stack _including_ "ilen", which we
> still have a pointer to. That means if the operation concludes after
> a timeout, we _will_ follow a rogue pointer.
> A couple of functions have this race condition.
Got it. Will fix that on next version.
> 
> 

Thanks
Ye Xiang