* [PATCH] PCI: prevent out of bounds access in numa_node override
@ 2015-10-04 21:49 Sasha Levin
2015-10-06 19:36 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2015-10-04 21:49 UTC (permalink / raw)
To: bhelgaas, prarit; +Cc: linux-pci, linux-kernel, Sasha Levin
Commit 63692df1 ("PCI: Allow numa_node override via sysfs") didn't check that
the numa node provided by userspace is valid. Passing a node number too high
would attempt to access invalid memory and trigger a kernel panic.
Fixes: 63692df1 ("PCI: Allow numa_node override via sysfs")
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
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 312f23a..e9abca8 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_online(node))
+ if (node > MAX_NUMNODES || !node_online(node))
return -EINVAL;
add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] PCI: prevent out of bounds access in numa_node override 2015-10-04 21:49 [PATCH] PCI: prevent out of bounds access in numa_node override Sasha Levin @ 2015-10-06 19:36 ` Bjorn Helgaas 2015-10-06 20:02 ` Prarit Bhargava 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2015-10-06 19:36 UTC (permalink / raw) To: Sasha Levin; +Cc: bhelgaas, prarit, linux-pci, linux-kernel Hi Sasha, On Sun, Oct 04, 2015 at 05:49:29PM -0400, Sasha Levin wrote: > Commit 63692df1 ("PCI: Allow numa_node override via sysfs") didn't check that > the numa node provided by userspace is valid. Passing a node number too high > would attempt to access invalid memory and trigger a kernel panic. > > Fixes: 63692df1 ("PCI: Allow numa_node override via sysfs") > Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > --- > 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 312f23a..e9abca8 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_online(node)) > + if (node > MAX_NUMNODES || !node_online(node)) This needs to be "node >= MAX_NUMNODES", doesn't it? I'll fix it up if you agree. Looks like a candidate for stable. > return -EINVAL; > > add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); > -- > 1.7.10.4 > > -- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: prevent out of bounds access in numa_node override 2015-10-06 19:36 ` Bjorn Helgaas @ 2015-10-06 20:02 ` Prarit Bhargava 2015-10-07 14:07 ` Sasha Levin 2015-10-07 16:09 ` Bjorn Helgaas 0 siblings, 2 replies; 6+ messages in thread From: Prarit Bhargava @ 2015-10-06 20:02 UTC (permalink / raw) To: Bjorn Helgaas, Sasha Levin; +Cc: bhelgaas, linux-pci, linux-kernel On 10/06/2015 03:36 PM, Bjorn Helgaas wrote: > Hi Sasha, > > On Sun, Oct 04, 2015 at 05:49:29PM -0400, Sasha Levin wrote: >> Commit 63692df1 ("PCI: Allow numa_node override via sysfs") didn't check that >> the numa node provided by userspace is valid. Passing a node number too high >> would attempt to access invalid memory and trigger a kernel panic. >> >> Fixes: 63692df1 ("PCI: Allow numa_node override via sysfs") >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >> --- >> 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 312f23a..e9abca8 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_online(node)) >> + if (node > MAX_NUMNODES || !node_online(node)) > > This needs to be "node >= MAX_NUMNODES", doesn't it? I'll fix it up if > you agree. Not a strenuous objection, but I don't see much bound checking using MAX_NUMNODES in the kernel outside of the core numa area. Is fixing node_online() with bounds checking a better option here so that other callers get the fix? I would have thought that calling node_online() with node > MAX_NUMNODES should be safe to call. P. > > Looks like a candidate for stable. > >> return -EINVAL; >> >> add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); >> -- >> 1.7.10.4 >> >> -- >> 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: prevent out of bounds access in numa_node override 2015-10-06 20:02 ` Prarit Bhargava @ 2015-10-07 14:07 ` Sasha Levin 2015-10-07 16:04 ` Bjorn Helgaas 2015-10-07 16:09 ` Bjorn Helgaas 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2015-10-07 14:07 UTC (permalink / raw) To: Prarit Bhargava, Bjorn Helgaas; +Cc: bhelgaas, linux-pci, linux-kernel On 10/06/2015 04:02 PM, Prarit Bhargava wrote: > > > On 10/06/2015 03:36 PM, Bjorn Helgaas wrote: >> Hi Sasha, >> >> On Sun, Oct 04, 2015 at 05:49:29PM -0400, Sasha Levin wrote: >>> Commit 63692df1 ("PCI: Allow numa_node override via sysfs") didn't check that >>> the numa node provided by userspace is valid. Passing a node number too high >>> would attempt to access invalid memory and trigger a kernel panic. >>> >>> Fixes: 63692df1 ("PCI: Allow numa_node override via sysfs") >>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> >>> --- >>> 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 312f23a..e9abca8 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_online(node)) >>> + if (node > MAX_NUMNODES || !node_online(node)) >> >> This needs to be "node >= MAX_NUMNODES", doesn't it? I'll fix it up if >> you agree. Yup, you're right. > > Not a strenuous objection, but I don't see much bound checking using > MAX_NUMNODES in the kernel outside of the core numa area. Is fixing > node_online() with bounds checking a better option here so that other callers > get the fix? I would have thought that calling node_online() with node > > MAX_NUMNODES should be safe to call. I don't know, this will add overhead to node_online(), and isn't really done in any other similar function. For example, cpu_online() isn't safe to call with cpu > NR_CPUS either. Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: prevent out of bounds access in numa_node override 2015-10-07 14:07 ` Sasha Levin @ 2015-10-07 16:04 ` Bjorn Helgaas 0 siblings, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2015-10-07 16:04 UTC (permalink / raw) To: Sasha Levin; +Cc: Prarit Bhargava, bhelgaas, linux-pci, linux-kernel On Wed, Oct 07, 2015 at 10:07:33AM -0400, Sasha Levin wrote: > > On 10/06/2015 03:36 PM, Bjorn Helgaas wrote: > >> Hi Sasha, > >> > >> On Sun, Oct 04, 2015 at 05:49:29PM -0400, Sasha Levin wrote: > >>> Commit 63692df1 ("PCI: Allow numa_node override via sysfs") didn't check that > >>> the numa node provided by userspace is valid. Passing a node number too high > >>> would attempt to access invalid memory and trigger a kernel panic. > >>> > >>> Fixes: 63692df1 ("PCI: Allow numa_node override via sysfs") > >>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > >>> --- > >>> 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 312f23a..e9abca8 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_online(node)) > >>> + if (node > MAX_NUMNODES || !node_online(node)) > >> > >> This needs to be "node >= MAX_NUMNODES", doesn't it? I'll fix it up if > >> you agree. > > Yup, you're right. OK, applied to for-linus for v4.3 as follows. Thanks a lot, Sasha! commit 0f7b9ad7477f7ce9b30e9ab0048f6e726f11a08a Author: Sasha Levin <sasha.levin@oracle.com> Date: Sun Oct 4 17:49:29 2015 -0400 PCI: Prevent out of bounds access in numa_node override 63692df103e9 ("PCI: Allow numa_node override via sysfs") didn't check that the numa node provided by userspace is valid. Passing a node number too high would attempt to access invalid memory and trigger a kernel panic. Fixes: 63692df103e9 ("PCI: Allow numa_node override via sysfs") Signed-off-by: Sasha Levin <sasha.levin@oracle.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> CC: stable@vger.kernel.org # v3.19+ diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 312f23a..9261868 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_online(node)) + if (node >= MAX_NUMNODES || !node_online(node)) return -EINVAL; add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: prevent out of bounds access in numa_node override 2015-10-06 20:02 ` Prarit Bhargava 2015-10-07 14:07 ` Sasha Levin @ 2015-10-07 16:09 ` Bjorn Helgaas 1 sibling, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2015-10-07 16:09 UTC (permalink / raw) To: Prarit Bhargava; +Cc: Sasha Levin, bhelgaas, linux-pci, linux-kernel Hi Prarit, On Tue, Oct 06, 2015 at 04:02:22PM -0400, Prarit Bhargava wrote: > On 10/06/2015 03:36 PM, Bjorn Helgaas wrote: > > On Sun, Oct 04, 2015 at 05:49:29PM -0400, Sasha Levin wrote: > >> Commit 63692df1 ("PCI: Allow numa_node override via sysfs") didn't check that > >> the numa node provided by userspace is valid. Passing a node number too high > >> would attempt to access invalid memory and trigger a kernel panic. > >> > >> Fixes: 63692df1 ("PCI: Allow numa_node override via sysfs") > >> Signed-off-by: Sasha Levin <sasha.levin@oracle.com> > >> --- > >> 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 312f23a..e9abca8 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_online(node)) > >> + if (node > MAX_NUMNODES || !node_online(node)) > > > > This needs to be "node >= MAX_NUMNODES", doesn't it? I'll fix it up if > > you agree. > > Not a strenuous objection, but I don't see much bound checking using > MAX_NUMNODES in the kernel outside of the core numa area. Is fixing > node_online() with bounds checking a better option here so that other callers > get the fix? I would have thought that calling node_online() with node > > MAX_NUMNODES should be safe to call. Yes, that would certainly be an option. I don't feel super strongly either way, but one argument in favor of Sasha's approach is that the validation of user input is nice and obvious right at the point where we process the input. Bjorn ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-07 16:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-04 21:49 [PATCH] PCI: prevent out of bounds access in numa_node override Sasha Levin 2015-10-06 19:36 ` Bjorn Helgaas 2015-10-06 20:02 ` Prarit Bhargava 2015-10-07 14:07 ` Sasha Levin 2015-10-07 16:04 ` Bjorn Helgaas 2015-10-07 16:09 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).