linux-kernel - Re: [PATCH 1/1] platform/x86/intel: Fix uninitialized entry in pmt_crashlog

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: <7d908ecc145f3dbf35161e39587f7bd82f1b64c2.camel@linux.intel.com>
Date:   Wed, 08 Jun 2022 08:56:22 -0700
From:   "David E. Box" <david.e.box@...ux.intel.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Xiaohui Zhang <xiaohuizhang@....edu.cn>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] platform/x86/intel: Fix uninitialized entry in
 pmt_crashlog_probe

On Wed, 2022-06-08 at 12:58 +0200, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 9:45 AM Xiaohui Zhang <xiaohuizhang@....edu.cn> wrote:
> > Similar to the handling of pmt_telem_probe in commit 2cdfa0c20d58
> > ("platform/x86/intel: Fix 'rmmod pmt_telemetry' panic"), we thought
> > a patch might be needed here as well.
> > 
> > The probe function, pmt_crashlog_probe(), adds an entry for devices even if
> > they have not been initialized.  This results in the array of initialized
> > devices containing both initialized and uninitialized entries.  This
> > causes a panic in the remove function, pmt_crashlog_remove() which expects
> > the array to only contain initialized entries.
> > 
> > Only use an entry when a device is initialized.

Thanks for the patch. I've already acked the same fix here:

https://lore.kernel.org/all/9df6d96f9672b16ca88f5d2c7dd43c87ae938ad3.camel@linux.intel.com/

> 
> ...
> 
> >         auxiliary_set_drvdata(auxdev, priv);
> > 
> >         for (i = 0; i < intel_vsec_dev->num_resources; i++) {
> > -               struct intel_pmt_entry *entry = &priv->entry[i].entry;
> > +               struct intel_pmt_entry *entry = &priv->entry[priv-
> > >num_entries].entry;
> 
> This requires more explanation, what do the entry and the i mean in
> the below call?

priv->entry is an array of crashlog devices. i is the index to a resource for a
single device. It is correct that i cannot also be used, as it was here, to
refer to the device in the array. This is because we will skip some devices we
detect as unsupported without failing probe. So the array only contains a list
of devices we've added and may be less than the total available. This doesn't
affect probe or device usage, but it did affect remove since the current code
left gaps in the array where devices were skipped.

> Why is using the same entry over all resources fine?
> num_entries as an index is pointing out of bounds, is that what was
> really expected (first free entry?)?

Not shown is num_entries++ which happens only after successful addition of an
entry. It's not the case that the same entry gets reused over all resources.

num_entries cannot be used out of bounds. The array is allocated with
struct_size to the total number of resources.

Thanks,

David
> 
> 
> >                 ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns,
> > intel_vsec_dev, i);
> >                 if (ret < 0)
> 
>