Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: "Sriram Dash" <sriram.dash@samsung.com>
To: "'Kishon Vijay Abraham I'" <kishon@ti.com>,
	"'Shradha Todi'" <shradha.t@samsung.com>
Cc: <lorenzo.pieralisi@arm.com>, <bhelgaas@google.com>,
	<pankaj.dubey@samsung.com>, <linux-pci@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] PCI: endpoint: Fix NULL pointer dereference for ->get_features()
Date: Thu, 2 Apr 2020 20:01:59 +0530
Message-ID: <000001d608fb$7ab39010$701ab030$@samsung.com> (raw)
In-Reply-To: <a7a6a295-160a-94d6-09f9-63f783c8b28a@ti.com>

> From: Kishon Vijay Abraham I <kishon@ti.com>
> Subject: Re: [PATCH] PCI: endpoint: Fix NULL pointer dereference for -
> >get_features()
> 
> Hi Sriram,
> 
> On 3/19/2020 7:06 PM, Sriram Dash wrote:
> >> From: Shradha Todi <shradha.t@samsung.com>
> >> Subject: [PATCH] PCI: endpoint: Fix NULL pointer dereference for -
> >>> get_features()
> >>
> >> get_features ops of pci_epc_ops may return NULL, causing NULL pointer
> >> dereference in pci_epf_test_bind function. Let us add a check for
> >> pci_epc_feature pointer in pci_epf_test_bind before we access it to
> >> avoid any such NULL pointer dereference and return -ENOTSUPP in case
> >> pci_epc_feature is not found.
> >>
> >> Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> >> Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
> >> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> >> ---
> >
> > Hi Kishon,
> >
> > Any update on this?
> 
> Don't we access epc_features only after checking if epc_features is not NULL in
> pci_epf_test_bind() function? However we are accessing epc_features in
> multiple other functions all over pci-epf-test.

We access the epc_feature after checking the NULL condition in the bind function.
However, we do not stop if the epc_feature is NULL and proceed for allocation in the
pci_epf_test_alloc_space function, for example. During this allocation, we do not check
for NULL condition for epc_feature and hence, if any controller driver is not providing
the epc features, it will panic accessing epc_features.

> 
> So the patch itself is correct though the commit log has to be fixed. You should
> also check if all the endpoint controller drivers existing currently provides
> epc_features.

At the moment, there is no issue for existing controller drivers as I can see almost
all drivers are providing epc_features. But, this is not a mandatory feature and some
controller drivers may not have epc_features implemented, may be in the near future.
But because we are dealing with the configfs, the application need not bother about
the driver details underneath.

IMO, the code should be fixed regardless and should not cause panic in any case.

> 
> Thanks
> Kishon
> >
> >
> >>  drivers/pci/endpoint/functions/pci-epf-test.c | 15 +++++++++------
> >>  1 file changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c
> >> b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> index c9121b1b9fa9..af4537a487bf 100644
> >> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> >> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> >> @@ -510,14 +510,17 @@ static int pci_epf_test_bind(struct pci_epf *epf)
> >>  		return -EINVAL;
> >>
> >>  	epc_features = pci_epc_get_features(epc, epf->func_no);
> >> -	if (epc_features) {
> >> -		linkup_notifier = epc_features->linkup_notifier;
> >> -		msix_capable = epc_features->msix_capable;
> >> -		msi_capable = epc_features->msi_capable;
> >> -		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
> >> -		pci_epf_configure_bar(epf, epc_features);
> >> +	if (!epc_features) {
> >> +		dev_err(dev, "epc_features not implemented\n");
> >> +		return -ENOTSUPP;
> >>  	}
> >>
> >> +	linkup_notifier = epc_features->linkup_notifier;
> >> +	msix_capable = epc_features->msix_capable;
> >> +	msi_capable = epc_features->msi_capable;
> >> +	test_reg_bar = pci_epc_get_first_free_bar(epc_features);
> >> +	pci_epf_configure_bar(epf, epc_features);
> >> +
> >>  	epf_test->test_reg_bar = test_reg_bar;
> >>  	epf_test->epc_features = epc_features;
> >>
> >> --
> >> 2.17.1
> >
> >


  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200311103443epcas5p2e97b8f3a8e52dc6f02eb551e0c97f132@epcas5p2.samsung.com>
2020-03-11 10:28 ` Shradha Todi
2020-03-19 13:36   ` Sriram Dash
2020-03-20 10:20     ` Kishon Vijay Abraham I
2020-04-02 14:31       ` Sriram Dash [this message]
2020-07-06 11:17         ` Lorenzo Pieralisi
2020-07-06 13:17           ` Sriram Dash

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='000001d608fb$7ab39010$701ab030$@samsung.com' \
    --to=sriram.dash@samsung.com \
    --cc=bhelgaas@google.com \
    --cc=kishon@ti.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=pankaj.dubey@samsung.com \
    --cc=shradha.t@samsung.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git