Re: [PATCH v3 0/3] Actually fix freelist pointer vs redzoning
On Thu, Oct 15, 2020 at 11:44:15AM +0200, Vlastimil Babka wrote: > On 10/15/20 10:23 AM, Christopher Lameter wrote: > > On Wed, 14 Oct 2020, Kees Cook wrote: > > > > > Note on patch 2: Christopher NAKed it, but I actually think this is a > > > reasonable thing to add -- the "too small" check is only made when built > > > with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip > > > over this directly, even if it would never make it into a released > > > kernel. I see no reason to just leave this foot-gun in place, though, so > > > we might as well just fix it too. (Which seems to be what Longman was > > > similarly supporting, IIUC.) > > > > Well then remove the duplication of checks. The NAK was there because it > > seems that you were not aware of the existing checks. > > > > > Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable, > > > and the other 2 can land. :) > > > > Just deal with the old checks too and it will be fine. > > Yeah, the existing check is under CONFIG_DEBUG_VM, which means it's not > active on some configurations. Creating a cache is not exactly fast path > operation, so I would remove this guard. > As for the minimum size check, I would probably remove it (but watch out if > SLAB/SLOB can handle it). It's not effective to use slab cache for 4-byte > objects, but why make it an error. Err, why did the check exist to begin with? If the check isn't wanted, that's one thing, but I was just trying to fix what I saw in the redzone handling. What is preferred here? 1) drop patch 2 2) keep patch 2, but also: a) validate slab/slob can handle < word-sized allocations b) remove check in kmem_cache_sanity_check option 2a seems like it could be fragile if I miss something. I think I'd rather just take option 1. -- Kees Cook
Re: [PATCH v3 0/3] Actually fix freelist pointer vs redzoning
On 10/15/20 10:23 AM, Christopher Lameter wrote: On Wed, 14 Oct 2020, Kees Cook wrote: Note on patch 2: Christopher NAKed it, but I actually think this is a reasonable thing to add -- the "too small" check is only made when built with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip over this directly, even if it would never make it into a released kernel. I see no reason to just leave this foot-gun in place, though, so we might as well just fix it too. (Which seems to be what Longman was similarly supporting, IIUC.) Well then remove the duplication of checks. The NAK was there because it seems that you were not aware of the existing checks. Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable, and the other 2 can land. :) Just deal with the old checks too and it will be fine. Yeah, the existing check is under CONFIG_DEBUG_VM, which means it's not active on some configurations. Creating a cache is not exactly fast path operation, so I would remove this guard. As for the minimum size check, I would probably remove it (but watch out if SLAB/SLOB can handle it). It's not effective to use slab cache for 4-byte objects, but why make it an error.
Re: [PATCH v3 0/3] Actually fix freelist pointer vs redzoning
On Wed, 14 Oct 2020, Kees Cook wrote: > Note on patch 2: Christopher NAKed it, but I actually think this is a > reasonable thing to add -- the "too small" check is only made when built > with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip > over this directly, even if it would never make it into a released > kernel. I see no reason to just leave this foot-gun in place, though, so > we might as well just fix it too. (Which seems to be what Longman was > similarly supporting, IIUC.) Well then remove the duplication of checks. The NAK was there because it seems that you were not aware of the existing checks. > Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable, > and the other 2 can land. :) Just deal with the old checks too and it will be fine.
[PATCH v3 0/3] Actually fix freelist pointer vs redzoning
v3: - fix commit messages to properly reflect the direction of the overwrite - justify the less-than-word-size patch better - add Acks - move some Fixes up into the commit log as just references v2: https://lore.kernel.org/lkml/20201009195411.4018141-1-keesc...@chromium.org v1: https://lore.kernel.org/lkml/20201008233443.3335464-1-keesc...@chromium.org This fixes redzoning vs the freelist pointer (both for middle-position and very small caches). Both are "theoretical" fixes, in that I see no evidence of such small-sized caches actually be used in the kernel, but that's no reason to let the bugs continue to exist. :) Note on patch 2: Christopher NAKed it, but I actually think this is a reasonable thing to add -- the "too small" check is only made when built with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip over this directly, even if it would never make it into a released kernel. I see no reason to just leave this foot-gun in place, though, so we might as well just fix it too. (Which seems to be what Longman was similarly supporting, IIUC.) Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable, and the other 2 can land. :) Thanks! -Kees Kees Cook (3): mm/slub: Clarify verification reporting mm/slub: Fix redzoning for small allocations mm/slub: Actually fix freelist pointer vs redzoning Documentation/vm/slub.rst | 10 +- mm/slub.c | 36 +++- 2 files changed, 20 insertions(+), 26 deletions(-) -- 2.25.1
