Re: Recent i2c-designware initialization fix

[Jean Delvare <jdelvare@suse.de>]

On Wed, 17 Sep 2025 12:46:32 +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Jean Delvare <jdelvare@suse.de>
> >Sent: Wednesday, September 17, 2025 7:05 AM
> >To: Ruhl, Michael J <michael.j.ruhl@intel.com>
> >Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Andi Shyti
> ><andi.shyti@kernel.org>; Jarkko Nikula <jarkko.nikula@linux.intel.com>; Mika
> >Westerberg <mika.westerberg@linux.intel.com>; Jan Dabros
> ><jsd@semihalf.com>; linux-i2c@vger.kernel.org
> >Subject: Recent i2c-designware initialization fix
> >
> >Hello Michael,
> >
> >I have been looking into your kernel commit 3d30048958e0
> >("i2c/designware: Fix an initialization issue"), as it received a CVE
> >number (CVE-2025-38380), I was asked to evaluate the severity of the
> >bug.
> >
> >I was not familiar with the i2c-designware driver so it took me some
> >reading time to form an opinion on the matter. But after a careful
> >review, my conclusion is that there was no actual bug in the first
> >place. If I'm correct then CVE-2025-38380 is invalid.
> >
> >My reasoning is as follows:
> >* struct dw_i2c_dev is allocated per-PCI device in i2c_dw_pci_probe()
> >  using devm_kzalloc(), therefore all its members, including
> >  dev->msg_write_idx, are initialized to 0.
> >* The supposedly problematic code path is only taken if (dev->flags &
> >  MODEL_MASK) == MODEL_AMD_NAVI_GPU.
> >* The only place where dev->msg_write_idx is set to a non-zero value is
> >  in i2c_dw_xfer_msg(). This function is only called by i2c_dw_isr(),
> >  which in turn is only called if the device is not in polling mode.
> >* The flags set for the AMD Navi GPU devices are:
> >  dev->flags |= MODEL_AMD_NAVI_GPU | ACCESS_POLLING
> >  so these devices are always in polling mode and never use an
> >  interrupt so i2c_dw_isr() is never called for them.
> >
> >If my reasoning is correct, then for the AMD Navi GPU devices,
> >dev->msg_write_idx is implicitly initialized to 0 at allocation time
> >and its value is never changed after that, so the explicit
> >initialization to 0 which has been added in amd_i2c_dw_xfer_quirk() is
> >a no-op.
> >
> >What do you think? Am I missing something?  
> 
> You are correct.
> 
> This is a preventive fix to have a common init path for the common data structure.
> 
> The concern being that if the code is updated to use this idx, without this additional init,
> the code will behave unexpectedly.

OK, I understand the rationale, thanks for clarifying.

Next time this happens, I suggest mentioning in the commit message that
the fix is in anticipation of future changes and not fixing an existing
bug. This will make it easier for kernel branch maintainers to decide
whether the fix needs to be backported, and for the security team to
decide whether or not to attribute a CVE number.

(As a side note, I don't think i2c_dw_xfer_init() should be using
dev->msg_write_idx in the first place, I'll submit an alternative fix,
but this should be discussed separately.)

> I am not terribly familiar with the CVE process, but from my understanding, I don't think
> that this is an issue that warrants one.

Unfortunately (from my perspective) kernel fixes can get a CVE number
automatically based on a some keywords. Your commit message mentioned a
potential out of bound array access, which falls into that category.

I'll let the kernel CVE team know that this CVE is invalid so that they
can cancel it.

-- 
Jean Delvare
SUSE L3 Support