linux-kernel - Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 17 Jun 2019 18:57:30 +0200
From:   Uladzislau Rezki <urezki@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Uladzislau Rezki <urezki@...il.com>, Roman Gushchin <guro@...com>,
        Michal Hocko <mhocko@...e.com>,
        Matthew Wilcox <willy@...radead.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Joel Fernandes <joelaf@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...e.hu>, Tejun Heo <tj@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Roman Penyaev <rpenyaev@...e.de>,
        Rick Edgecombe <rick.p.edgecombe@...el.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [BUG]: mm/vmalloc: uninitialized variable access in
 pcpu_get_vm_areas

> 
> I managed to un-confuse gcc-8 by turning the if/else if/else into
> a switch statement. If you all think this is an acceptable solution,
> I'll submit that after some more testing to ensure it addresses
> all configurations:
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index a9213fc3802d..5b7e50de008b 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -915,7 +915,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>  {
>         struct vmap_area *lva;
> 
> -       if (type == FL_FIT_TYPE) {
> +       switch (type) {
> +       case FL_FIT_TYPE:
>                 /*
>                  * No need to split VA, it fully fits.
>                  *
> @@ -925,7 +926,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  */
>                 unlink_va(va, &free_vmap_area_root);
>                 kmem_cache_free(vmap_area_cachep, va);
> -       } else if (type == LE_FIT_TYPE) {
> +               break;
> +       case LE_FIT_TYPE:
>                 /*
>                  * Split left edge of fit VA.
>                  *
> @@ -934,7 +936,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  * |-------|-------|
>                  */
>                 va->va_start += size;
> -       } else if (type == RE_FIT_TYPE) {
> +               break;
> +       case RE_FIT_TYPE:
>                 /*
>                  * Split right edge of fit VA.
>                  *
> @@ -943,7 +946,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  * |-------|-------|
>                  */
>                 va->va_end = nva_start_addr;
> -       } else if (type == NE_FIT_TYPE) {
> +               break;
> +       case NE_FIT_TYPE:
>                 /*
>                  * Split no edge of fit VA.
>                  *
> @@ -980,7 +984,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  * Shrink this VA to remaining size.
>                  */
>                 va->va_start = nva_start_addr + size;
> -       } else {
> +               break;
> +       default:
>                 return -1;
>         }
> 
To me it is not clear how it would solve the warning. It sounds like
your GCC after this change is able to keep track of that variable
probably because of less generated code. But i am not sure about
other versions. For example i have:

gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)

and it totally OK, i.e. it does not emit any related warning.

Another thing is that, if we add mode code there or change the function
prototype, we might run into the same warning. Therefore i proposed that
we just set the variable to NULL, i.e. Initialize it.

<snip>
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index b1bb5fc6eb05..10cfb93aba1e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -913,7 +913,11 @@ adjust_va_to_fit_type(struct vmap_area *va,
        unsigned long nva_start_addr, unsigned long size,
        enum fit_type type)
 {
-       struct vmap_area *lva;
+       /*
+        * Some GCC versions can emit bogus warning that it
+        * may be used uninitialized, therefore set it NULL.
+        */
+       struct vmap_area *lva = NULL;
 
        if (type == FL_FIT_TYPE) {
                /*
<snip>

--
Vlad Rezki