| Message ID | 1447003444-27108-1-git-send-email-minipli@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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. -- 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
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 -- 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
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 > -- 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
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);
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(-)