From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755749AbcGHQIG (ORCPT ); Fri, 8 Jul 2016 12:08:06 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:38346 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755448AbcGHQH6 (ORCPT ); Fri, 8 Jul 2016 12:07:58 -0400 MIME-Version: 1.0 In-Reply-To: References: <577f7e55.4668420a.84f17.5cb9SMTPIN_ADDED_MISSING@mx.google.com> From: Kees Cook Date: Fri, 8 Jul 2016 12:07:54 -0400 X-Google-Sender-Auth: BziarfrENP9PrFiruawQaoAZZW0 Message-ID: Subject: Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support To: Christoph Lameter Cc: Michael Ellerman , "kernel-hardening@lists.openwall.com" , Jan Kara , Catalin Marinas , Will Deacon , Linux-MM , sparclinux , linux-ia64@vger.kernel.org, Andrea Arcangeli , linux-arch , "x86@kernel.org" , Russell King , PaX Team , Borislav Petkov , lin , Mathias Krause , Fenghua Yu , Rik van Riel , David Rientjes , Tony Luck , Andy Lutomirski , Joonsoo Kim , Dmitry Vyukov , Laura Abbott , Brad Spengler , Ard Biesheuvel , LKML , Pekka Enberg , Case y Schauf ler , Andrew Morton , "linuxppc-dev@lists.ozlabs.org" , "David S. Miller" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter wrote: > On Fri, 8 Jul 2016, Michael Ellerman wrote: > >> > I wonder if this code should be using size_from_object() instead of s->size? BTW, I can't reproduce this on x86 yet... >> >> Hmm, not sure. Who's SLUB maintainer? :) > > Me. > > s->size is the size of the whole object including debugging info etc. > ksize() gives you the actual usable size of an object. Is check_valid_pointer() making sure the pointer is within the usable size? It seemed like it was checking that it was within the slub object (checks against s->size, wants it above base after moving pointer to include redzone, etc). I think a potential problem with Michael's fix is that the ptr in __check_heap_object() may not point at the _start_ of the usable object, so doing the red zone shift isn't quite right. This finds the ptr's offset within the slub object (since s->size is the slub object size): offset = (ptr - page_address(page)) % s->size; But this looks at object_size and doesn't take into account actual size: if (offset <= s->object_size && n <= s->object_size - offset) return NULL; I think offset needs to be adjusted by the size of padding, which the restore_red_left() call had the same effect, but may not cover all padding conditions? I'm not sure. Should it be: /* Find offset within slab object. */ offset = (ptr - page_address(page)) % s->size; /* Adjust offset for meta data and padding. */ offset -= s->size - s->object_size; /* Make sure offset and size are within bounds of the allocation size. */ if (offset <= s->object_size && n <= s->object_size - offset) return NULL; ? -Kees -- Kees Cook Chrome OS & Brillo Security From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Date: Fri, 8 Jul 2016 12:07:54 -0400 Message-ID: References: <577f7e55.4668420a.84f17.5cb9SMTPIN_ADDED_MISSING@mx.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: linux-ia64-owner@vger.kernel.org To: Christoph Lameter Cc: Michael Ellerman , "kernel-hardening@lists.openwall.com" , Jan Kara , Catalin Marinas , Will Deacon , Linux-MM , sparclinux , linux-ia64@vger.kernel.org, Andrea Arcangeli , linux-arch , "x86@kernel.org" , Russell King , PaX Team , Borislav Petkov , lin , Mathias Krause , Fenghua Yu , Rik van Riel , David Rientjes , Tony Luck , Andy Lutomirski , Joonsoo Kim , Dmitry Vyukov , Laura List-Id: linux-arch.vger.kernel.org On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter wrote: > On Fri, 8 Jul 2016, Michael Ellerman wrote: > >> > I wonder if this code should be using size_from_object() instead of s->size? BTW, I can't reproduce this on x86 yet... >> >> Hmm, not sure. Who's SLUB maintainer? :) > > Me. > > s->size is the size of the whole object including debugging info etc. > ksize() gives you the actual usable size of an object. Is check_valid_pointer() making sure the pointer is within the usable size? It seemed like it was checking that it was within the slub object (checks against s->size, wants it above base after moving pointer to include redzone, etc). I think a potential problem with Michael's fix is that the ptr in __check_heap_object() may not point at the _start_ of the usable object, so doing the red zone shift isn't quite right. This finds the ptr's offset within the slub object (since s->size is the slub object size): offset = (ptr - page_address(page)) % s->size; But this looks at object_size and doesn't take into account actual size: if (offset <= s->object_size && n <= s->object_size - offset) return NULL; I think offset needs to be adjusted by the size of padding, which the restore_red_left() call had the same effect, but may not cover all padding conditions? I'm not sure. Should it be: /* Find offset within slab object. */ offset = (ptr - page_address(page)) % s->size; /* Adjust offset for meta data and padding. */ offset -= s->size - s->object_size; /* Make sure offset and size are within bounds of the allocation size. */ if (offset <= s->object_size && n <= s->object_size - offset) return NULL; ? -Kees -- Kees Cook Chrome OS & Brillo Security From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f41.google.com ([74.125.82.41]:38347 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755172AbcGHQH5 (ORCPT ); Fri, 8 Jul 2016 12:07:57 -0400 Received: by mail-wm0-f41.google.com with SMTP id n127so15849405wme.1 for ; Fri, 08 Jul 2016 09:07:56 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <577f7e55.4668420a.84f17.5cb9SMTPIN_ADDED_MISSING@mx.google.com> From: Kees Cook Date: Fri, 8 Jul 2016 12:07:54 -0400 Message-ID: Subject: Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Content-Type: text/plain; charset=UTF-8 Sender: linux-arch-owner@vger.kernel.org List-ID: To: Christoph Lameter Cc: Michael Ellerman , "kernel-hardening@lists.openwall.com" , Jan Kara , Catalin Marinas , Will Deacon , Linux-MM , sparclinux , linux-ia64@vger.kernel.org, Andrea Arcangeli , linux-arch , "x86@kernel.org" , Russell King , PaX Team , Borislav Petkov , lin , Mathias Krause , Fenghua Yu , Rik van Riel , David Rientjes , Tony Luck , Andy Lutomirski , Joonsoo Kim , Dmitry Vyukov , Laura Abbott , Brad Spengler , Ard Biesheuvel , LKML , Pekka Enberg , Case y Schauf ler , Andrew Morton , "linuxppc-dev@lists.ozlabs.org" , "David S. Miller" Message-ID: <20160708160754.9H6ld9WEHlWU2AOzic-S4kIY1wYGwdLjP35M93fwDb8@z> On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter wrote: > On Fri, 8 Jul 2016, Michael Ellerman wrote: > >> > I wonder if this code should be using size_from_object() instead of s->size? BTW, I can't reproduce this on x86 yet... >> >> Hmm, not sure. Who's SLUB maintainer? :) > > Me. > > s->size is the size of the whole object including debugging info etc. > ksize() gives you the actual usable size of an object. Is check_valid_pointer() making sure the pointer is within the usable size? It seemed like it was checking that it was within the slub object (checks against s->size, wants it above base after moving pointer to include redzone, etc). I think a potential problem with Michael's fix is that the ptr in __check_heap_object() may not point at the _start_ of the usable object, so doing the red zone shift isn't quite right. This finds the ptr's offset within the slub object (since s->size is the slub object size): offset = (ptr - page_address(page)) % s->size; But this looks at object_size and doesn't take into account actual size: if (offset <= s->object_size && n <= s->object_size - offset) return NULL; I think offset needs to be adjusted by the size of padding, which the restore_red_left() call had the same effect, but may not cover all padding conditions? I'm not sure. Should it be: /* Find offset within slab object. */ offset = (ptr - page_address(page)) % s->size; /* Adjust offset for meta data and padding. */ offset -= s->size - s->object_size; /* Make sure offset and size are within bounds of the allocation size. */ if (offset <= s->object_size && n <= s->object_size - offset) return NULL; ? -Kees -- Kees Cook Chrome OS & Brillo Security From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Date: Fri, 08 Jul 2016 16:07:54 +0000 Subject: Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Message-Id: List-Id: References: <577f7e55.4668420a.84f17.5cb9SMTPIN_ADDED_MISSING@mx.google.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Christoph Lameter Cc: Michael Ellerman , "kernel-hardening@lists.openwall.com" , Jan Kara , Catalin Marinas , Will Deacon , Linux-MM , sparclinux , linux-ia64@vger.kernel.org, Andrea Arcangeli , linux-arch , "x86@kernel.org" , Russell King , PaX Team , Borislav Petkov , lin , Mathias Krause , Fenghua Yu , Rik van Riel , David Rientjes , Tony Luck , Andy Lutomirski , Joonsoo Kim , Dmitry Vyukov , Laura Abbott , Brad Spengler , Ard Biesheuvel , LKML , Pekka Enberg , Case y Schauf ler , Andrew Morton , "linuxppc-dev@lists.ozlabs.org" , "David S. Miller" On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter wrote: > On Fri, 8 Jul 2016, Michael Ellerman wrote: > >> > I wonder if this code should be using size_from_object() instead of s->size? BTW, I can't reproduce this on x86 yet... >> >> Hmm, not sure. Who's SLUB maintainer? :) > > Me. > > s->size is the size of the whole object including debugging info etc. > ksize() gives you the actual usable size of an object. Is check_valid_pointer() making sure the pointer is within the usable size? It seemed like it was checking that it was within the slub object (checks against s->size, wants it above base after moving pointer to include redzone, etc). I think a potential problem with Michael's fix is that the ptr in __check_heap_object() may not point at the _start_ of the usable object, so doing the red zone shift isn't quite right. This finds the ptr's offset within the slub object (since s->size is the slub object size): offset = (ptr - page_address(page)) % s->size; But this looks at object_size and doesn't take into account actual size: if (offset <= s->object_size && n <= s->object_size - offset) return NULL; I think offset needs to be adjusted by the size of padding, which the restore_red_left() call had the same effect, but may not cover all padding conditions? I'm not sure. Should it be: /* Find offset within slab object. */ offset = (ptr - page_address(page)) % s->size; /* Adjust offset for meta data and padding. */ offset -= s->size - s->object_size; /* Make sure offset and size are within bounds of the allocation size. */ if (offset <= s->object_size && n <= s->object_size - offset) return NULL; ? -Kees -- Kees Cook Chrome OS & Brillo Security From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f70.google.com (mail-wm0-f70.google.com [74.125.82.70]) by kanga.kvack.org (Postfix) with ESMTP id 3D3856B0005 for ; Fri, 8 Jul 2016 12:07:57 -0400 (EDT) Received: by mail-wm0-f70.google.com with SMTP id n127so15530669wme.1 for ; Fri, 08 Jul 2016 09:07:57 -0700 (PDT) Received: from mail-wm0-x236.google.com (mail-wm0-x236.google.com. [2a00:1450:400c:c09::236]) by mx.google.com with ESMTPS id r123si3411776wmb.115.2016.07.08.09.07.55 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Jul 2016 09:07:56 -0700 (PDT) Received: by mail-wm0-x236.google.com with SMTP id n127so15849404wme.1 for ; Fri, 08 Jul 2016 09:07:55 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <577f7e55.4668420a.84f17.5cb9SMTPIN_ADDED_MISSING@mx.google.com> From: Kees Cook Date: Fri, 8 Jul 2016 12:07:54 -0400 Message-ID: Subject: Re: [kernel-hardening] Re: [PATCH 9/9] mm: SLUB hardened usercopy support Content-Type: text/plain; charset=UTF-8 Sender: owner-linux-mm@kvack.org List-ID: To: Christoph Lameter Cc: Michael Ellerman , "kernel-hardening@lists.openwall.com" , Jan Kara , Catalin Marinas , Will Deacon , Linux-MM , sparclinux , linux-ia64@vger.kernel.org, Andrea Arcangeli , linux-arch , "x86@kernel.org" , Russell King , PaX Team , Borislav Petkov , lin , Mathias Krause , Fenghua Yu , Rik van Riel , David Rientjes , Tony Luck , Andy Lutomirski , Joonsoo Kim , Dmitry Vyukov , Laura Abbott , Brad Spengler , Ard Biesheuvel , LKML , Pekka Enberg , Case y Schauf ler , Andrew Morton , "linuxppc-dev@lists.ozlabs.org" , "David S. Miller" On Fri, Jul 8, 2016 at 9:45 AM, Christoph Lameter wrote: > On Fri, 8 Jul 2016, Michael Ellerman wrote: > >> > I wonder if this code should be using size_from_object() instead of s->size? BTW, I can't reproduce this on x86 yet... >> >> Hmm, not sure. Who's SLUB maintainer? :) > > Me. > > s->size is the size of the whole object including debugging info etc. > ksize() gives you the actual usable size of an object. Is check_valid_pointer() making sure the pointer is within the usable size? It seemed like it was checking that it was within the slub object (checks against s->size, wants it above base after moving pointer to include redzone, etc). I think a potential problem with Michael's fix is that the ptr in __check_heap_object() may not point at the _start_ of the usable object, so doing the red zone shift isn't quite right. This finds the ptr's offset within the slub object (since s->size is the slub object size): offset = (ptr - page_address(page)) % s->size; But this looks at object_size and doesn't take into account actual size: if (offset <= s->object_size && n <= s->object_size - offset) return NULL; I think offset needs to be adjusted by the size of padding, which the restore_red_left() call had the same effect, but may not cover all padding conditions? I'm not sure. Should it be: /* Find offset within slab object. */ offset = (ptr - page_address(page)) % s->size; /* Adjust offset for meta data and padding. */ offset -= s->size - s->object_size; /* Make sure offset and size are within bounds of the allocation size. */ if (offset <= s->object_size && n <= s->object_size - offset) return NULL; ? -Kees -- Kees Cook Chrome OS & Brillo Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org