linux-kernel - Re: [PATCH v1 5/5] vfio/pci: Drop duplicate check for size parameter of memremap()

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191115155214.55e949cc@x1.home>
Date:   Fri, 15 Nov 2019 15:52:14 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-kernel@...r.kernel.org, Christoph Hellwig <hch@....de>,
        Dan Williams <dan.j.williams@...el.com>,
        Cornelia Huck <cohuck@...hat.com>
Subject: Re: [PATCH v1 5/5] vfio/pci: Drop duplicate check for size
 parameter of memremap()

On Fri, 15 Nov 2019 20:00:44 +0200
Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

> Since memremap() returns NULL pointer for size = 0, there is no need
> to duplicate this check in the callers.
> 
> Cc: Christoph Hellwig <hch@....de>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Cc: Alex Williamson <alex.williamson@...hat.com>
> Cc: Cornelia Huck <cohuck@...hat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/vfio/pci/vfio_pci_igd.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_igd.c
> index 53d97f459252..3088a33af271 100644
> --- a/drivers/vfio/pci/vfio_pci_igd.c
> +++ b/drivers/vfio/pci/vfio_pci_igd.c
> @@ -75,13 +75,7 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_device *vdev)
>  		return -EINVAL;
>  	}
>  
> -	size = le32_to_cpu(*(__le32 *)(base + 16));
> -	if (!size) {
> -		memunmap(base);
> -		return -EINVAL;
> -	}
> -
> -	size *= 1024; /* In KB */
> +	size = le32_to_cpu(*(__le32 *)(base + 16)) * 1024; /* In KB */
>  
>  	if (size != OPREGION_SIZE) {
>  		memunmap(base);

This seems convoluted, patch 1/5 states "[t]here is no use of memremap()
to be called with size = 0", which we weren't doing thanks to the check
removed above.  So now we are potentially calling it with zero,
apparently only to take advantage of this new behavior, and we lose the
error granularity that previously such a condition failed with an
-EINVAL and now we fail with an -ENONMEM and cannot distinguish whether
the OpRegion table size was empty or we just weren't able to memremap()
it.  I don't see how this is an improvement.  Thanks,

Alex