Date:   Tue, 14 Mar 2023 15:15:05 +0800
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
Message-ID: <ZBAe+bpKmvC1iVfq@ye-NUC7i7DNHE>
References: <20230309071100.2856899-1-xiang.ye@intel.com>
 <20230309071100.2856899-2-xiang.ye@intel.com>
 <097d1742-1aa8-fc0b-db11-3571ea49383d@suse.com>
 <ZAqukC7XfSbIa9ov@ye-NUC7i7DNHE>
 <ca995ed7-e6db-4265-e528-5e29fb418594@suse.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ca995ed7-e6db-4265-e528-5e29fb418594@suse.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?mXPr3HhatrpyGmFhTFUAxfHThdj10Xh/1JC4T9WMCdVpQEqx4GEdnNQjoSW4?=
 =?us-ascii?Q?z/dmIG7Q9SRlvWcDbm9gGNPysaJb9frxvO2AMTqjNvBpL/00e77tHx8gcu/z?=
 =?us-ascii?Q?K2PoBdrf8LKaDXWRC1HkGpNNgFHQcvZyw7I3NIpMAh3tfh5NT70soxX7zCQo?=
 =?us-ascii?Q?ornqhWxKMRNYuDySg/q/tlW3qZU0pHXqPlZ4U/F8h7Rfx+8Q89EmkGWPjrxR?=
 =?us-ascii?Q?IxIpY14UtFu6wNwfIxggYYbjN/tL3eh1ME21z8wCL2m0v+H2kS/za9RaZXBD?=
 =?us-ascii?Q?ekss3Qw6pU8s4vPMW5NfxVKrjzw0szAmFO/+sJXIyvgL+UsqszlZYpJWjxgZ?=
 =?us-ascii?Q?Iz13qLMxATJl83v7UAFzS08qvAYwH3rs99ZXgA+5GYcCJdUzFh5YoAcobSEQ?=
 =?us-ascii?Q?yeY6qNTfddx60zhBpSuygqXFSIZZOkOt1Ki6x7QtBi8bppag56S962EIi6RZ?=
 =?us-ascii?Q?xkAwAO9F4b1zfcO/LNvjVGYv5CJMaJmIZc609/UcxubHWAGevcIrQTTgHaBC?=
 =?us-ascii?Q?JaeOrK1cOAeyKOkM6+X08CZLkb2zDqfvl00wtZrdOXy5tSD7f56WoW804VHx?=
 =?us-ascii?Q?Fbl30HJZqe7vsezbaiZToWEZdHktvYfn28/LDW9yX/ckCuG0QHpC1V2d1+tQ?=
 =?us-ascii?Q?0DiEHe1Ywbvym82Q7rkn9Nv/um+xfzyriikSecweGVz0VGkjvmTAQFkq8Gpf?=
 =?us-ascii?Q?mgMlH9CBkycsv61CKFZvczedwNSRpDvoOnBk07lzj3tnRVM104tVt6nJI+GE?=
 =?us-ascii?Q?1rx2sCcHTKCdr2HNZLceALz/bFZHOlZQGrVyeTn/yHxaoXTHs3NVjjjZepL8?=
 =?us-ascii?Q?omN6iRZrqpVtbUZHDBdAI0Os/n68/8985UxMB2Xize5Z25OeJYKQWsBueutV?=
 =?us-ascii?Q?GV85wUfMZKAuKV1idqMl3pNdTK7fGQStWz3GhL6buTAfmaf73/M1hiW+1PqA?=
 =?us-ascii?Q?h/mjSM7MHF3+vDzx8Tsy4DJCQC/CaQgFq2i0WoYJhsp4D5NVFVuLsdn/+BsC?=
 =?us-ascii?Q?cX/3Hqy/CUCSCaKfyH5awHGfp2lwxzmu0x+vrTMw0QevpOAo/wfyn3vFzGZ7?=
 =?us-ascii?Q?w0mb2BlA4VZCGb5zsmyxRHKxHggZXWoEoDQ3ogr5Md0C07V6azCL2TuKdjTy?=
 =?us-ascii?Q?Z2DKu7tkSBFGw2yJul9lWWIxPsmUxMZEtioqvs6+9sAvr/pm4bufa1XlwcQ4?=
 =?us-ascii?Q?mzxy4OtN3qSf+THHCe0WH07u4TM+YxXIlnv39ELLJiC6rOZ+w07xSWMiGwYw?=
 =?us-ascii?Q?xw/ZPUfpx2aH7W1WxTo3N5oH0dGRsqKmhcIDJZ2bhXfaq2qCHUO9VnQsbVKO?=
 =?us-ascii?Q?XfUOiAQAi1cqKFJtlj/5pwMTsF/bhOYSTnPcHJn79cbZ1PZ/iYvvCC3n8ZzU?=
 =?us-ascii?Q?CjoseqhbLN3Eq7C6TBAMPDgvw71tcwv3bmP/VX9B4T14FrJ+QXIELE2lZXq4?=
 =?us-ascii?Q?XeveJNaM4pPx7/jz16eMiJc23zuu9+laHspninaEZtvncfP+aa50pwPOsNJP?=
 =?us-ascii?Q?6t8kBP9UDTiTG/Ll1V0PWE2kVhHim8vt0t8IKyM0ej/95N6oTohGmQim/T54?=
 =?us-ascii?Q?CmZ1CVlPabr6d1KFN+Rcq4Jy5g+HfQLtasKrZgrB?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 80b5cea4-2d04-4b76-c0e2-08db245bd96b
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1418.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2023 07:15:11.4210
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jnpKXN/Ir1FHhTIYrGzY7o4tsFwzRHPS1Pzd7FZqR+CqNdFTnygGJb+jsMlvuiKzn3M3EVmB7YBbl+dlj6L6sQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5920
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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