Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <202004151054.BD695840@keescook>
In-Reply-To: <202004151054.BD695840@keescook>
From:   Marco Elver <elver@google.com>
Date:   Wed, 15 Apr 2020 20:07:57 +0200
Message-ID: <CANpmjNPORrwPkqByhtKgVoh5=KHkpYHM0zRv6O1RgONmx3ZqOg@mail.gmail.com>
Subject: Re: [PATCH] slub: Avoid redzone when choosing freepointer location
To:     Kees Cook <keescook@chromium.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 15 Apr 2020 at 19:55, Kees Cook <keescook@chromium.org> wrote:
>
> Marco Elver reported system crashes when booting with "slub_debug=Z".
> The freepointer location (s->offset) was not taking into account that
> the "inuse" size that includes the redzone area should not be used by
> the freelist pointer. Change the calculation to save the area of the
> object that an inline freepointer may be written into.
>
> Reported-by: Marco Elver <elver@google.com>
> Link: https://lore.kernel.org/linux-mm/20200415164726.GA234932@google.com
> Fixes: 3202fa62fb43 ("slub: relocate freelist pointer to middle of object")
> Signed-off-by: Kees Cook <keescook@chromium.org>

Works for me, thank you!

Tested-by: Marco Elver <elver@google.com>

> ---
>  mm/slub.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 332d4b459a90..9bf44955c4f1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3533,6 +3533,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
>  {
>         slab_flags_t flags = s->flags;
>         unsigned int size = s->object_size;
> +       unsigned int freepointer_area;
>         unsigned int order;
>
>         /*
> @@ -3541,6 +3542,13 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
>          * the possible location of the free pointer.
>          */
>         size = ALIGN(size, sizeof(void *));
> +       /*
> +        * This is the area of the object where a freepointer can be
> +        * safely written. If redzoning adds more to the inuse size, we
> +        * can't use that portion for writing the freepointer, so
> +        * s->offset must be limited within this for the general case.
> +        */
> +       freepointer_area = size;
>
>  #ifdef CONFIG_SLUB_DEBUG
>         /*
> @@ -3582,13 +3590,13 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
>                  */
>                 s->offset = size;
>                 size += sizeof(void *);
> -       } else if (size > sizeof(void *)) {
> +       } else if (freepointer_area > sizeof(void *)) {
>                 /*
>                  * Store freelist pointer near middle of object to keep
>                  * it away from the edges of the object to avoid small
>                  * sized over/underflows from neighboring allocations.
>                  */
> -               s->offset = ALIGN(size / 2, sizeof(void *));
> +               s->offset = ALIGN(freepointer_area / 2, sizeof(void *));
>         }
>
>  #ifdef CONFIG_SLUB_DEBUG
> --
> 2.20.1
>
>
> --
> Kees Cook