Re: [PATCH v9 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: NULL pointer deref in pci_perf_show() if FMB allocation fails
- [High] s390/pci: NULL pointer deref if FMB allocation fails during enable
--

commit edcda811c60da07b857039d32e74cbe9c76de00e
Author: Omar Elghoul <oelghoul@linux.ibm.com>

s390/pci: Reuse FMB buffer and preserve state in device re-enablement

Don't free the FMB buffer when disabling measurement in
zpci_fmb_disable_device(). Instead, make the buffer persistent for the
lifetime of the device and reuse it across enable/disable cycles. Defer
freeing the buffer until teardown in zpci_release_device().

> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index c055a9ad09724..bda91d79bafdc 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -167,43 +167,13 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
>  /* Modify PCI: Set PCI function measurement parameters */
>  int zpci_fmb_enable_device(struct zpci_dev *zdev)
>  {
>  	lockdep_assert_held(&zdev->fmb_lock);
>  
> -	if (zdev->fmb || sizeof(*zdev->fmb) < zdev->fmb_length)
> +	if (zdev->fmb_requested || sizeof(*zdev->fmb) < zdev->fmb_length)
>  		return -EINVAL;
>  
> +	zdev->fmb_requested = true;
> +	return zpci_fmb_reenable_device(zdev);
>  }

[Severity: High]
If zpci_fmb_reenable_device() fails, is zdev->fmb_requested left set to
true? If the allocation in zpci_fmb_reenable_device() returns -ENOMEM, it
appears this state change is not rolled back.

[ ... ]

> +int zpci_fmb_reenable_device(struct zpci_dev *zdev)
> +{
[ ... ]
> +	if (!zdev->fmb) {
> +		zdev->fmb = kmem_cache_zalloc(zdev_fmb_cache, GFP_KERNEL);
> +		if (!zdev->fmb)
> +			return -ENOMEM;
> +	} else {
> +		/* reuse the same FMB buffer for as long the zdev lives */
> +		memset(zdev->fmb, 0, sizeof(*zdev->fmb));
>  	}

If kmem_cache_zalloc() fails here, zdev->fmb remains NULL, but the caller
zpci_fmb_enable_device() has already set zdev->fmb_requested = true.

> diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
> index c7ed7bf254b5e..9a960310d4ddf 100644
> --- a/arch/s390/pci/pci_debug.c
> +++ b/arch/s390/pci/pci_debug.c
> @@ -97,7 +97,7 @@ static int pci_perf_show(struct seq_file *m, void *v)
>  		return 0;
>  
>  	mutex_lock(&zdev->fmb_lock);
> -	if (!zdev->fmb) {
> +	if (!zdev->fmb_requested) {
>  		mutex_unlock(&zdev->fmb_lock);
>  		seq_puts(m, "FMB statistics disabled\n");
>  		return 0;

[Severity: High]
If fmb_requested is left true after an allocation failure, can this
lead to a NULL pointer dereference here?

Since this check now relies entirely on fmb_requested rather than checking
if zdev->fmb is actually allocated, it seems like pci_perf_show() will bypass
this early return and proceed to dereference the NULL zdev->fmb pointer
further down:

arch/s390/pci/pci_debug.c:pci_perf_show() {
	...
	/* header */
	seq_printf(m, "Update interval: %u ms\n", zdev->fmb_update);
	seq_printf(m, "Samples: %u\n", zdev->fmb->samples);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007135926.82935-1-oelghoul@linux.ibm.com?part=2