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

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: <ZBAe+bpKmvC1iVfq@ye-NUC7i7DNHE>
Date:   Tue, 14 Mar 2023 15:15:05 +0800
From:   "Ye, Xiang" <xiang.ye@...el.com>
To:     Oliver Neukum <oneukum@...e.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Matthias Kaehlcke <mka@...omium.org>,
        Lee Jones <lee@...nel.org>, Wolfram Sang <wsa@...nel.org>,
        Tyrone Ting <kfting@...oton.com>,
        Mark Brown <broonie@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        <linux-usb@...r.kernel.org>, <linux-i2c@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <linux-gpio@...r.kernel.org>, <srinivas.pandruvada@...el.com>,
        <heikki.krogerus@...ux.intel.com>,
        <andriy.shevchenko@...ux.intel.com>,
        <sakari.ailus@...ux.intel.com>, <zhifeng.wang@...el.com>,
        <wentong.wu@...el.com>, <lixu.zhang@...el.com>
Subject: Re: [PATCH v4 1/5] mfd: Add support for Intel LJCA device

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