Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <7d908ecc145f3dbf35161e39587f7bd82f1b64c2.camel@linux.intel.com>
Subject: Re: [PATCH 1/1] platform/x86/intel: Fix uninitialized entry in
 pmt_crashlog_probe
From:   "David E. Box" <david.e.box@linux.intel.com>
Reply-To: david.e.box@linux.intel.com
To:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Xiaohui Zhang <xiaohuizhang@ruc.edu.cn>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Platform Driver <platform-driver-x86@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Date:   Wed, 08 Jun 2022 08:56:22 -0700
In-Reply-To: <CAHp75VenPBtQJ7SVMaOLqER_5HjUDUT-bdJawb3LK=n9oU+4Yg@mail.gmail.com>
References: <20220608011712.2371-1-xiaohuizhang@ruc.edu.cn>
         <CAHp75VenPBtQJ7SVMaOLqER_5HjUDUT-bdJawb3LK=n9oU+4Yg@mail.gmail.com>
Organization: David E. Box
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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