[PATCH] PCI: Prevent out of bounds access in numa_node override - part 2

[PATCH] PCI: Prevent out of bounds access in numa_node override - part 2

[Mathias Krause]

Commit 1266963170f5 ("PCI: Prevent out of bounds access in numa_node
override") missed that the user provided node could also be negative.
Handle this case as well to really avoid out-of-bounds accesses to
the node_states[] array.

Fixes: 1266963170f5 ("PCI: Prevent out of bounds access in numa_node...")
Signed-off-by: Mathias Krause <minipli@googlemail.com>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Prarit Bhargava <prarit@redhat.com>
CC: stable@vger.kernel.org	# v3.19+
---
 drivers/pci/pci-sysfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 92618686604c..87835d50b927 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -216,7 +216,7 @@ static ssize_t numa_node_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (node >= MAX_NUMNODES || !node_online(node))
+	if (node < 0 || node >= MAX_NUMNODES || !node_online(node))
 		return -EINVAL;
 
 	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
-- 
1.7.10.4

Re: [PATCH] PCI: Prevent out of bounds access in numa_node override - part 2

[Prarit Bhargava]

On 11/08/2015 12:24 PM, Mathias Krause wrote:
> Commit 1266963170f5 ("PCI: Prevent out of bounds access in numa_node
> override") missed that the user provided node could also be negative.
> Handle this case as well to really avoid out-of-bounds accesses to
> the node_states[] array.

No, this is incorrect.  More often than not, numa_node is -1 for NUMA_NO_NODE
which is often interpreted in the kernel as "any numa node".

[root@intel-brickland-04 pci0000:ff]# find ./ -name *numa_node* | xargs egrep ^
| egrep "\-1" | wc -l
92

Can you point to the code that does node_states[pci_dev->numa_node] without
doing a bounds check?  IMO that's the code that is broken.

FWIW: I think the idea of your patch is still correct.  Checking for -1 to
MAX_NUMNODES is not a bad idea.

P.

Re: [PATCH] PCI: Prevent out of bounds access in numa_node override - part 2

[Mathias Krause]

On 9 November 2015 at 00:28, Prarit Bhargava <prarit@redhat.com> wrote:
> On 11/08/2015 12:24 PM, Mathias Krause wrote:
>> Commit 1266963170f5 ("PCI: Prevent out of bounds access in numa_node
>> override") missed that the user provided node could also be negative.
>> Handle this case as well to really avoid out-of-bounds accesses to
>> the node_states[] array.
>
> No, this is incorrect.  More often than not, numa_node is -1 for NUMA_NO_NODE
> which is often interpreted in the kernel as "any numa node".
>
> [root@intel-brickland-04 pci0000:ff]# find ./ -name *numa_node* | xargs egrep ^
> | egrep "\-1" | wc -l
> 92

While this is correct, the patch does not hinder this. It just
prevents *changing* the value to a negative one using the sysfs
interface.

Looking at the initial patch, introducing that interface in commit
63692df103e9 ("PCI: Allow numa_node override via sysfs"), I suspect
it's meant to "fix" broken BIOSes by providing the correct NUMA
topology by hand by writing NUMA node numbers to the corresponding
numa_node sysfs files. Reverting back to 'no specific node' might be a
valid use case. I'll change the patch to allow -1, i.e. NUMA_NO_NODE,
too.

> Can you point to the code that does node_states[pci_dev->numa_node] without
> doing a bounds check?  IMO that's the code that is broken.

It's the node_state() inline for MAX_NUMNODES > 1.

>
> FWIW: I think the idea of your patch is still correct.  Checking for -1 to
> MAX_NUMNODES is not a bad idea.

It is. As it prevents userland from triggering the out of bounds read. ;)


Thanks,
Mathias

Re: [PATCH] PCI: Prevent out of bounds access in numa_node override - part 2

[Prarit Bhargava]

On 11/09/2015 01:56 AM, Mathias Krause wrote:

> 
>> Can you point to the code that does node_states[pci_dev->numa_node] without
>> doing a bounds check?  IMO that's the code that is broken.
> 
> It's the node_state() inline for MAX_NUMNODES > 1.

In drivers/pci/pci-sysfs.c: numa_node_store()

	if (node >= MAX_NUMNODES || !node_online(node))

needs to be broken out into a range and separate online check.

	/* range check */
	if (node < NUMA_NO_NODE || node >= MAX_NUMNODES)
		return -EINVAL;

	/* Is the specific node online? */
	if (node != NUMA_NO_NODE && !node_online(node))
		return -EINVAL; /* perhaps -ENODEV ? */

which will fix the problem.

P.

> 
>>
>> FWIW: I think the idea of your patch is still correct.  Checking for -1 to
>> MAX_NUMNODES is not a bad idea.
> 
> It is. As it prevents userland from triggering the out of bounds read. ;)
> 
> 
> Thanks,
> Mathias
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>