[PATCH 1/1] platform/x86/intel: Fix uninitialized entry in pmt_crashlog_probe

[PATCH 1/1] platform/x86/intel: Fix uninitialized entry in pmt_crashlog_probe

[Xiaohui Zhang]

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.

Signed-off-by: Xiaohui Zhang <xiaohuizhang@ruc.edu.cn>
---
 drivers/platform/x86/intel/pmt/crashlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
index 34daf9df168b..ace1239bc0a0 100644
--- a/drivers/platform/x86/intel/pmt/crashlog.c
+++ b/drivers/platform/x86/intel/pmt/crashlog.c
@@ -282,7 +282,7 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
 	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;
 
 		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, intel_vsec_dev, i);
 		if (ret < 0)
-- 
2.17.1

Re: [PATCH 1/1] platform/x86/intel: Fix uninitialized entry in pmt_crashlog_probe

[Andy Shevchenko]

On Wed, Jun 8, 2022 at 9:45 AM Xiaohui Zhang <xiaohuizhang@ruc.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.

...

>         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? 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?)?


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


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] platform/x86/intel: Fix uninitialized entry in pmt_crashlog_probe

[David E. Box]

On Wed, 2022-06-08 at 12:58 +0200, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 9:45 AM Xiaohui Zhang <xiaohuizhang@ruc.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)
> 
>