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

[PATCH v2] 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 avoid out-of-bounds accesses to the
node_states[] array.  However, allow the special value -1, i.e.
NUMA_NO_NODE, to be able to set the 'no specific node' configuration.

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+
---
v2: allow NUMA_NO_NODE

 drivers/pci/pci-sysfs.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

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

[Bjorn Helgaas]

On Mon, Nov 09, 2015 at 08:00:27PM +0100, 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 avoid out-of-bounds accesses to the
> node_states[] array.  However, allow the special value -1, i.e.
> NUMA_NO_NODE, to be able to set the 'no specific node' configuration.
> 
> 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+

Applied as tweaked below to for-linus for v4.4, thanks!  As written,
if NUMA_NO_NODE were defined as -2, we would incorrectly accept -1.
Let me know if you disagree with my fix.

> ---
> v2: allow NUMA_NO_NODE
> 
>  drivers/pci/pci-sysfs.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 92618686604c..6e9818227b19 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -216,7 +216,10 @@ static ssize_t numa_node_store(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	if (node >= MAX_NUMNODES || !node_online(node))
> +	if (node < NUMA_NO_NODE || node >= MAX_NUMNODES)
> +		return -EINVAL;
> +
> +	if (node != NUMA_NO_NODE && !node_online(node))
>  		return -EINVAL;
>  
>  	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);



commit 2a35194c5a45fbb9ca1d88bc56804dfb51a75233
Author: Mathias Krause <minipli@googlemail.com>
Date:   Mon Nov 9 20:00:27 2015 +0100

    PCI: Prevent out of bounds access in numa_node override
    
    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 avoid out-of-bounds accesses to the
    node_states[] array.  However, allow the special value -1, i.e.
    NUMA_NO_NODE, to be able to set the 'no specific node' configuration.
    
    [bhelgaas: remove assumption that NUMA_NO_NODE == -1]
    Fixes: 1266963170f5 ("PCI: Prevent out of bounds access in numa_node override")
    Fixes: 63692df103e9 ("PCI: Allow numa_node override via sysfs")
    Signed-off-by: Mathias Krause <minipli@googlemail.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    CC: Sasha Levin <sasha.levin@oracle.com>
    CC: Prarit Bhargava <prarit@redhat.com>
    CC: stable@vger.kernel.org	# v3.19+

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9261868..50f4747 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -216,7 +216,12 @@ 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) {
+		if (node != NUMA_NO_NODE)
+			return -EINVAL;
+	}
+
+	if (node != NUMA_NO_NODE && !node_online(node))
 		return -EINVAL;
 
 	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);

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

[Mathias Krause]

On 24 November 2015 at 19:49, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Applied as tweaked below to for-linus for v4.4, thanks!  As written,
> if NUMA_NO_NODE were defined as -2, we would incorrectly accept -1.
> Let me know if you disagree with my fix.

I don't think the value of NUMA_NO_NODE will (or even has to) ever
change, as we're already exporting that value to userland via sysfs.
But you're right, the code shouldn't make any assumptions about the
concrete value of NUMA_NO_NODE and just handle it as a special
symbolic value.

> commit 2a35194c5a45fbb9ca1d88bc56804dfb51a75233
> Author: Mathias Krause <minipli@googlemail.com>
> Date:   Mon Nov 9 20:00:27 2015 +0100
>
>     PCI: Prevent out of bounds access in numa_node override
>
>     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 avoid out-of-bounds accesses to the
>     node_states[] array.  However, allow the special value -1, i.e.
>     NUMA_NO_NODE, to be able to set the 'no specific node' configuration.
>
>     [bhelgaas: remove assumption that NUMA_NO_NODE == -1]
>     Fixes: 1266963170f5 ("PCI: Prevent out of bounds access in numa_node override")
>     Fixes: 63692df103e9 ("PCI: Allow numa_node override via sysfs")
>     Signed-off-by: Mathias Krause <minipli@googlemail.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     CC: Sasha Levin <sasha.levin@oracle.com>
>     CC: Prarit Bhargava <prarit@redhat.com>
>     CC: stable@vger.kernel.org  # v3.19+
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 9261868..50f4747 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -216,7 +216,12 @@ 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) {
> +               if (node != NUMA_NO_NODE)
> +                       return -EINVAL;
> +       }

I would have written something like this:

    if ((node < 0 && node != NUMA_NO_NODE) || node >= MAX_NUMNODES)
        return -EINVAL;

It's more compact, but your solution is essentially the same, so I'm
fine with it.

> +
> +       if (node != NUMA_NO_NODE && !node_online(node))
>                 return -EINVAL;
>
>         add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);

Thanks,
Mathias

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

[Bjorn Helgaas]

On Tue, Nov 24, 2015 at 08:27:04PM +0100, Mathias Krause wrote:
> On 24 November 2015 at 19:49, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > Applied as tweaked below to for-linus for v4.4, thanks!  As written,
> > if NUMA_NO_NODE were defined as -2, we would incorrectly accept -1.
> > Let me know if you disagree with my fix.
> 
> I don't think the value of NUMA_NO_NODE will (or even has to) ever
> change, as we're already exporting that value to userland via sysfs.
> But you're right, the code shouldn't make any assumptions about the
> concrete value of NUMA_NO_NODE and just handle it as a special
> symbolic value.
> 
> > commit 2a35194c5a45fbb9ca1d88bc56804dfb51a75233
> > Author: Mathias Krause <minipli@googlemail.com>
> > Date:   Mon Nov 9 20:00:27 2015 +0100
> >
> >     PCI: Prevent out of bounds access in numa_node override
> >
> >     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 avoid out-of-bounds accesses to the
> >     node_states[] array.  However, allow the special value -1, i.e.
> >     NUMA_NO_NODE, to be able to set the 'no specific node' configuration.
> >
> >     [bhelgaas: remove assumption that NUMA_NO_NODE == -1]
> >     Fixes: 1266963170f5 ("PCI: Prevent out of bounds access in numa_node override")
> >     Fixes: 63692df103e9 ("PCI: Allow numa_node override via sysfs")
> >     Signed-off-by: Mathias Krause <minipli@googlemail.com>
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >     CC: Sasha Levin <sasha.levin@oracle.com>
> >     CC: Prarit Bhargava <prarit@redhat.com>
> >     CC: stable@vger.kernel.org  # v3.19+
> >
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 9261868..50f4747 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -216,7 +216,12 @@ 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) {
> > +               if (node != NUMA_NO_NODE)
> > +                       return -EINVAL;
> > +       }
> 
> I would have written something like this:
> 
>     if ((node < 0 && node != NUMA_NO_NODE) || node >= MAX_NUMNODES)
>         return -EINVAL;

I adopted that, thanks!