netdev - Re: [net-next 1/7] net/sched: Check for null dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 27 Oct 2017 08:28:00 -0700
From:   Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
To:     Davide Caratti <dcaratti@...hat.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        jogreene@...hat.com, Ivan Vecera <ivecera@...hat.com>
Subject: Re: [net-next 1/7] net/sched: Check for null dev_queue on create flow

Hi Davide,


On 10/27/2017 03:23 AM, Davide Caratti wrote:
> On Thu, 2017-10-26 at 10:17 -0700, Jeff Kirsher wrote:
>> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
>>
>> In qdisc_alloc() the dev_queue pointer was used without any checks
>> being performed. If qdisc_create() gets a null dev_queue pointer, it
>> just passes it along to qdisc_alloc(), leading to a crash. That
>> happens if a root qdisc implements select_queue() and returns a null
>> dev_queue pointer for an "invalid handle", for example, or if the
>> dev_queue associated with the parent qdisc is null.
>>
>> This patch is in preparation for the next in this series, where
>> select_queue() is being added to mqprio and as it may return a null
>> dev_queue.
>>
>> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
>> Tested-by: Henrik Austad <henrik@...tad.us>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>> ---
>>  net/sched/sch_generic.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> hello,
> 
> I didn't notice this when I posted https://www.spinics.net/lists/netdev/ms
> g462986.html , about one hour later, targeting Dave's net tree. I saw
> similar issues, but in my setup dev_queue was a valid pointer, and dev was
> NULL. This made qdisc_alloc() dereference NULL, when accessing dev->
> members in the function body before returning the newly allocated qdisc.
> 
> So, in my understanding both tests are necessary, but a (very trivial)
> conflict will be generated when these two commits will be eventually
> merged together.
> 
> I like this suggestion from Ivan:
> 
> -- >8 --
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index bf8c81e07c70..6bd1ae993326 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -603,8 +603,14 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
>         struct Qdisc *sch;
>         unsigned int size = QDISC_ALIGN(sizeof(*sch)) + ops->priv_size;
>         int err = -ENOBUFS;
> -       struct net_device *dev = dev_queue->dev;
> +       struct net_device *dev;
> +
> +       if (!dev_queue || !dev_queue->dev) {
> +               err = !dev_queue ? -EINVAL : -ENOENT;
> +               goto errout;
> +       }
> 
> +       dev = dev_queue->dev;
>         p = kzalloc_node(size, GFP_KERNEL,
>                          netdev_queue_numa_node_read(dev_queue));
> 
> -- 8< --
> 
> and I volunteer for sending a v2 of 'net/sched: fix NULL pointer
> dereference in qdisc_alloc()' including this, targeting 'net' tree, with
> an appropriate tag. WDYT?


Thanks for reaching out!

The patches look independent to me, and in a way they tell different stories.
The conflict should be trivial, so I would just merge both separately and handle
it, but I'm fine with any approaches, really, so I'd go with whichever makes the
maintainers' lives easier.


Regards,
Jesus


> 
> regards,
>