linux-kernel - Re: [PATCH v2 09/34] misc: xlink-pcie: lh: Add PCIe EPF driver for Local Host

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]

Date:   Wed, 20 Jan 2021 18:57:59 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     mgross@...ux.intel.com
Cc:     markgross@...nel.org, arnd@...db.de, bp@...e.de,
        damien.lemoal@....com, dragan.cvetic@...inx.com, corbet@....net,
        leonard.crestez@....com, palmerdabbelt@...gle.com,
        paul.walmsley@...ive.com, peng.fan@....com, robh+dt@...nel.org,
        shawnguo@...nel.org, jassisinghbrar@...il.com,
        linux-kernel@...r.kernel.org,
        Srikanth Thokala <srikanth.thokala@...el.com>,
        Derek Kiernan <derek.kiernan@...inx.com>
Subject: Re: [PATCH v2 09/34] misc: xlink-pcie: lh: Add PCIe EPF driver for
 Local Host

On Fri, Jan 08, 2021 at 01:25:35PM -0800, mgross@...ux.intel.com wrote:
> From: Srikanth Thokala <srikanth.thokala@...el.com>
> 
> Add PCIe EPF driver for local host (lh) to configure BAR's and other
> HW resources. Underlying PCIe HW controller is a Synopsys DWC PCIe core.
> 
> Cc: Derek Kiernan <derek.kiernan@...inx.com>
> Cc: Dragan Cvetic <dragan.cvetic@...inx.com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Reviewed-by: Mark Gross <mgross@...ux.intel.com>
> Signed-off-by: Srikanth Thokala <srikanth.thokala@...el.com>
> ---
>  MAINTAINERS                                 |   6 +
>  drivers/misc/Kconfig                        |   1 +
>  drivers/misc/Makefile                       |   1 +
>  drivers/misc/xlink-pcie/Kconfig             |   9 +
>  drivers/misc/xlink-pcie/Makefile            |   1 +
>  drivers/misc/xlink-pcie/local_host/Makefile |   2 +
>  drivers/misc/xlink-pcie/local_host/epf.c    | 413 ++++++++++++++++++++
>  drivers/misc/xlink-pcie/local_host/epf.h    |  39 ++
>  drivers/misc/xlink-pcie/local_host/xpcie.h  |  38 ++

Why such a deep directory tree?  Why is "local_host" needed?

Anyway, one thing stood out instantly:

> +static int intel_xpcie_check_bar(struct pci_epf *epf,
> +				 struct pci_epf_bar *epf_bar,
> +				 enum pci_barno barno,
> +				 size_t size, u8 reserved_bar)
> +{
> +	if (reserved_bar & (1 << barno)) {
> +		dev_err(&epf->dev, "BAR%d is already reserved\n", barno);
> +		return -EFAULT;

That error is only allowed when you really have a fault from
reading/writing to/from userspace memory.  Not this type of foolish
programming error by the caller.

> +	}
> +
> +	if (epf_bar->size != 0 && epf_bar->size < size) {
> +		dev_err(&epf->dev, "BAR%d fixed size is not enough\n", barno);
> +		return -ENOMEM;

Did you really run out of memory or was the parameters sent to you
incorrect?  -EINVAL is the properly thing here, right?



> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_xpcie_configure_bar(struct pci_epf *epf,
> +				     const struct pci_epc_features
> +					*epc_features)

Odd indentation :(

> +{
> +	struct pci_epf_bar *epf_bar;
> +	bool bar_fixed_64bit;
> +	int ret, i;
> +
> +	for (i = BAR_0; i <= BAR_5; i++) {
> +		epf_bar = &epf->bar[i];
> +		bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
> +		if (bar_fixed_64bit)
> +			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> +		if (epc_features->bar_fixed_size[i])
> +			epf_bar->size = epc_features->bar_fixed_size[i];
> +
> +		if (i == BAR_2) {
> +			ret = intel_xpcie_check_bar(epf, epf_bar, BAR_2,
> +						    BAR2_MIN_SIZE,
> +						    epc_features->reserved_bar);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (i == BAR_4) {
> +			ret = intel_xpcie_check_bar(epf, epf_bar, BAR_4,
> +						    BAR4_MIN_SIZE,
> +						    epc_features->reserved_bar);
> +			if (ret)
> +				return ret;
> +		}

Why do you need to check all of this?  Where is the data coming from
that could be incorrect?

thanks,

greg k-h