From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.9 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BD430C5517A for ; Wed, 11 Nov 2020 18:29:43 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BDFBF21534 for ; Wed, 11 Nov 2020 18:29:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="eUBuV6ws" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BDFBF21534 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id E3B5C6B0036; Wed, 11 Nov 2020 13:29:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DEA0B6B005D; Wed, 11 Nov 2020 13:29:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D00966B0068; Wed, 11 Nov 2020 13:29:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0179.hostedemail.com [216.40.44.179]) by kanga.kvack.org (Postfix) with ESMTP id A37416B0036 for ; Wed, 11 Nov 2020 13:29:41 -0500 (EST) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 3BBA28249980 for ; Wed, 11 Nov 2020 18:29:41 +0000 (UTC) X-FDA: 77472975762.18.wall02_311313a272ff Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin18.hostedemail.com (Postfix) with ESMTP id BFD16100ED3A4 for ; Wed, 11 Nov 2020 18:29:40 +0000 (UTC) X-HE-Tag: wall02_311313a272ff X-Filterd-Recvd-Size: 16064 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by imf18.hostedemail.com (Postfix) with ESMTP for ; Wed, 11 Nov 2020 18:29:39 +0000 (UTC) Received: by mail-wr1-f66.google.com with SMTP id 33so3484408wrl.7 for ; Wed, 11 Nov 2020 10:29:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=gkKctNxP2ohF55zVwpehmRMz8y0vnC/mVPskztCYL/A=; b=eUBuV6wsfVCXPWMfqP96WZQE379N/uRWoeiNoC35/6K9nSNR/fE1JobllCUuVK13gy Yh/qSn2tsSi5Fph1pyJFwLsh1+9mys7dBXUUyeKPzG8KzoPk3KpQyasbfgpEOokTKfGN 0tHeADat7173U0HrmdTTT6k4QFbzOFKxBLAt0PliH9Fxsv21udSoXTLohQkAlmMv+DQw 01Plnut3Rs5ptmvio+XjfB1cjss6EHP7x81086fvViocfWT10a41JtTjNG6jFq2PvUBA U58sOFTOBYcoVCs+Jw9mtVXV6DI4dD/X0eF4hEIOvS9UwgO9kIGUcV8FDgvO1yiQEuno dJJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=gkKctNxP2ohF55zVwpehmRMz8y0vnC/mVPskztCYL/A=; b=WubcDrk2m2RjwIONMQboWJektBs9KnlHb1xYkDGFb+gsh1Mi6TIEvpw8WizRus1B5L vJFWe0JNghk17Xhxw/s2Kt13WCknW18afwe6TqZyCnl6YFZrARVBspe8Xds7STuMgebz kJX+E6c6TOZmUobasyQ5dNGQEqr+hhy+imTaeaLI4Mvi3VIpGAQaUx5mbjXSAWXk7g2o vTGRr8RszMbCxJVrZHb5B3XXj8uYFz6POL0g1910vzuDxyrZNUc4JsVxUGOyymXAOb6E 6MBX34AnawGI8TxbhM3DO3BOhXvnbCuxndLZbhFtPKofX6yRTJDgM7FlP7J3kxCfubgR 0ZqA== X-Gm-Message-State: AOAM530GaytAhLYnBj5UVtqaDqYjCdkwwSUyqTQcl1ujXLVsBJtI8e8X VusS8ESYmZGWYoaBaVtGPPp84g== X-Google-Smtp-Source: ABdhPJxePqv8gSeOmuGOWnBfUMw5qNq+rV7plAkkRNMIAaKPFSh/jL608gjTCnCrLyAC9Gt1Mn4TNA== X-Received: by 2002:a5d:518e:: with SMTP id k14mr34061180wrv.60.1605119378323; Wed, 11 Nov 2020 10:29:38 -0800 (PST) Received: from elver.google.com ([2a00:79e0:15:13:f693:9fff:fef4:2449]) by smtp.gmail.com with ESMTPSA id v19sm3612388wrf.40.2020.11.11.10.29.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Nov 2020 10:29:37 -0800 (PST) Date: Wed, 11 Nov 2020 19:29:31 +0100 From: Marco Elver To: Andrey Konovalov Cc: Dmitry Vyukov , Alexander Potapenko , Catalin Marinas , Will Deacon , Vincenzo Frascino , Evgenii Stepanov , Andrey Ryabinin , Branislav Rankov , Kevin Brodsky , Andrew Morton , kasan-dev@googlegroups.com, linux-arm-kernel@lists.infradead.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 11/20] kasan: add and integrate kasan boot parameters Message-ID: <20201111182931.GM517454@elver.google.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.14.6 (2020-07-11) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Nov 10, 2020 at 11:20PM +0100, 'Andrey Konovalov' via kasan-dev wrote: > Hardware tag-based KASAN mode is intended to eventually be used in > production as a security mitigation. Therefore there's a need for finer > control over KASAN features and for an existence of a kill switch. > > This change adds a few boot parameters for hardware tag-based KASAN that > allow to disable or otherwise control particular KASAN features. > > The features that can be controlled are: > > 1. Whether KASAN is enabled at all. > 2. Whether KASAN collects and saves alloc/free stacks. > 3. Whether KASAN panics on a detected bug or not. > > With this change a new boot parameter kasan.mode allows to choose one of > three main modes: > > - kasan.mode=off - KASAN is disabled, no tag checks are performed > - kasan.mode=prod - only essential production features are enabled > - kasan.mode=full - all KASAN features are enabled > > The chosen mode provides default control values for the features mentioned > above. However it's also possible to override the default values by > providing: > > - kasan.stacktrace=off/on - enable alloc/free stack collection > (default: on for mode=full, otherwise off) > - kasan.fault=report/panic - only report tag fault or also panic > (default: report) > > If kasan.mode parameter is not provided, it defaults to full when > CONFIG_DEBUG_KERNEL is enabled, and to prod otherwise. > > It is essential that switching between these modes doesn't require > rebuilding the kernel with different configs, as this is required by > the Android GKI (Generic Kernel Image) initiative [1]. > > [1] https://source.android.com/devices/architecture/kernel/generic-kernel-image > > Signed-off-by: Andrey Konovalov > Link: https://linux-review.googlesource.com/id/If7d37003875b2ed3e0935702c8015c223d6416a4 > --- > mm/kasan/common.c | 22 +++++-- > mm/kasan/hw_tags.c | 152 +++++++++++++++++++++++++++++++++++++++++++++ > mm/kasan/kasan.h | 16 +++++ > mm/kasan/report.c | 14 ++++- > 4 files changed, 197 insertions(+), 7 deletions(-) > > diff --git a/mm/kasan/common.c b/mm/kasan/common.c > index 4598c1364f19..efad5ed6a3bd 100644 > --- a/mm/kasan/common.c > +++ b/mm/kasan/common.c > @@ -129,6 +129,11 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, > unsigned int redzone_size; > int redzone_adjust; > > + if (!kasan_stack_collection_enabled()) { > + *flags |= SLAB_KASAN; > + return; > + } > + > /* Add alloc meta. */ > cache->kasan_info.alloc_meta_offset = *size; > *size += sizeof(struct kasan_alloc_meta); > @@ -165,6 +170,8 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, > > size_t kasan_metadata_size(struct kmem_cache *cache) > { > + if (!kasan_stack_collection_enabled()) > + return 0; > return (cache->kasan_info.alloc_meta_offset ? > sizeof(struct kasan_alloc_meta) : 0) + > (cache->kasan_info.free_meta_offset ? > @@ -267,11 +274,13 @@ void * __must_check kasan_init_slab_obj(struct kmem_cache *cache, > { > struct kasan_alloc_meta *alloc_meta; > > - if (!(cache->flags & SLAB_KASAN)) > - return (void *)object; > + if (kasan_stack_collection_enabled()) { > + if (!(cache->flags & SLAB_KASAN)) > + return (void *)object; > > - alloc_meta = kasan_get_alloc_meta(cache, object); > - __memset(alloc_meta, 0, sizeof(*alloc_meta)); > + alloc_meta = kasan_get_alloc_meta(cache, object); > + __memset(alloc_meta, 0, sizeof(*alloc_meta)); > + } > > if (IS_ENABLED(CONFIG_KASAN_SW_TAGS) || IS_ENABLED(CONFIG_KASAN_HW_TAGS)) > object = set_tag(object, assign_tag(cache, object, true, false)); > @@ -308,6 +317,9 @@ static bool __kasan_slab_free(struct kmem_cache *cache, void *object, > rounded_up_size = round_up(cache->object_size, KASAN_GRANULE_SIZE); > kasan_poison_memory(object, rounded_up_size, KASAN_KMALLOC_FREE); > > + if (!kasan_stack_collection_enabled()) > + return false; > + > if ((IS_ENABLED(CONFIG_KASAN_GENERIC) && !quarantine) || > unlikely(!(cache->flags & SLAB_KASAN))) > return false; > @@ -355,7 +367,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object, > kasan_poison_memory((void *)redzone_start, redzone_end - redzone_start, > KASAN_KMALLOC_REDZONE); > > - if (cache->flags & SLAB_KASAN) > + if (kasan_stack_collection_enabled() && (cache->flags & SLAB_KASAN)) > set_alloc_info(cache, (void *)object, flags); > > return set_tag(object, tag); > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c > index 838b29e44e32..2f6f0261af8c 100644 > --- a/mm/kasan/hw_tags.c > +++ b/mm/kasan/hw_tags.c > @@ -8,6 +8,8 @@ > > #define pr_fmt(fmt) "kasan: " fmt > > +#include > +#include This should include -- although the rest of the kernel seems to also inconsistently use on or the other. Since the name, as referred to also by macros are "static keys", perhaps the static_key.h header is more appropriate... > #include > #include > #include > @@ -17,9 +19,104 @@ > > #include "kasan.h" > > +enum kasan_arg_mode { > + KASAN_ARG_MODE_DEFAULT, > + KASAN_ARG_MODE_OFF, > + KASAN_ARG_MODE_PROD, > + KASAN_ARG_MODE_FULL, > +}; > + > +enum kasan_arg_stacktrace { > + KASAN_ARG_STACKTRACE_DEFAULT, It seems KASAN_ARG_STACKTRACE_DEFAULT is never used explicitly. Could the switch statements just be changed to not have a 'default' but instead refer to *DEFAULT where appropriate? > + KASAN_ARG_STACKTRACE_OFF, > + KASAN_ARG_STACKTRACE_ON, > +}; > + > +enum kasan_arg_fault { > + KASAN_ARG_FAULT_DEFAULT, > + KASAN_ARG_FAULT_REPORT, > + KASAN_ARG_FAULT_PANIC, > +}; > + > +static enum kasan_arg_mode kasan_arg_mode __ro_after_init; > +static enum kasan_arg_stacktrace kasan_arg_stacktrace __ro_after_init; > +static enum kasan_arg_fault kasan_arg_fault __ro_after_init; > + > +/* Whether KASAN is enabled at all. */ > +DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_enabled); > +EXPORT_SYMBOL(kasan_flag_enabled); > + > +/* Whether to collect alloc/free stack traces. */ > +DEFINE_STATIC_KEY_FALSE_RO(kasan_flag_stacktrace); > + > +/* Whether panic or disable tag checking on fault. */ > +bool kasan_flag_panic __ro_after_init; > + > +/* kasan.mode=off/prod/full */ > +static int __init early_kasan_mode(char *arg) > +{ > + if (!arg) > + return -EINVAL; > + > + if (!strcmp(arg, "off")) > + kasan_arg_mode = KASAN_ARG_MODE_OFF; > + else if (!strcmp(arg, "prod")) > + kasan_arg_mode = KASAN_ARG_MODE_PROD; > + else if (!strcmp(arg, "full")) > + kasan_arg_mode = KASAN_ARG_MODE_FULL; > + else > + return -EINVAL; > + > + return 0; > +} > +early_param("kasan.mode", early_kasan_mode); > + > +/* kasan.stack=off/on */ > +static int __init early_kasan_flag_stacktrace(char *arg) > +{ > + if (!arg) > + return -EINVAL; > + > + if (!strcmp(arg, "off")) > + kasan_arg_stacktrace = KASAN_ARG_STACKTRACE_OFF; > + else if (!strcmp(arg, "on")) > + kasan_arg_stacktrace = KASAN_ARG_STACKTRACE_ON; > + else > + return -EINVAL; > + > + return 0; > +} > +early_param("kasan.stacktrace", early_kasan_flag_stacktrace); > + > +/* kasan.fault=report/panic */ > +static int __init early_kasan_fault(char *arg) > +{ > + if (!arg) > + return -EINVAL; > + > + if (!strcmp(arg, "report")) > + kasan_arg_fault = KASAN_ARG_FAULT_REPORT; > + else if (!strcmp(arg, "panic")) > + kasan_arg_fault = KASAN_ARG_FAULT_PANIC; > + else > + return -EINVAL; > + > + return 0; > +} > +early_param("kasan.fault", early_kasan_fault); > + > /* kasan_init_hw_tags_cpu() is called for each CPU. */ > void kasan_init_hw_tags_cpu(void) > { > + /* > + * There's no need to check that the hardware is MTE-capable here, > + * as this function is only called for MTE-capable hardware. > + */ > + > + /* If KASAN is disabled, do nothing. */ > + if (kasan_arg_mode == KASAN_ARG_MODE_OFF) > + return; > + > hw_init_tags(KASAN_TAG_MAX); > hw_enable_tagging(); > } > @@ -27,6 +124,61 @@ void kasan_init_hw_tags_cpu(void) > /* kasan_init_hw_tags() is called once on boot CPU. */ > void kasan_init_hw_tags(void) Is this an __init function, since it sets __ro_after_init vars? > { > + /* If hardware doesn't support MTE, do nothing. */ > + if (!system_supports_mte()) > + return; > + > + /* If KASAN is disabled, do nothing. */ > + if (kasan_arg_mode == KASAN_ARG_MODE_OFF) > + return; This is checked twice, once here and the in the switch. I think remove the one here ^^^. > + /* Choose KASAN mode if kasan boot parameter is not provided. */ > + if (kasan_arg_mode == KASAN_ARG_MODE_DEFAULT) { > + if (IS_ENABLED(CONFIG_DEBUG_KERNEL)) > + kasan_arg_mode = KASAN_ARG_MODE_FULL; > + else > + kasan_arg_mode = KASAN_ARG_MODE_PROD; > + } > + > + /* Preset parameter values based on the mode. */ > + switch (kasan_arg_mode) { > + case KASAN_ARG_MODE_OFF: > + return; > + case KASAN_ARG_MODE_PROD: > + static_branch_enable(&kasan_flag_enabled); > + break; > + case KASAN_ARG_MODE_FULL: > + static_branch_enable(&kasan_flag_enabled); > + static_branch_enable(&kasan_flag_stacktrace); > + break; > + default: I'd suggest removing the 'default' cases in all the switches, so that we get warnings in case we add new options and they aren't handled. Here, having KASAN_ARG_MODE_DEFAULT is probably redundant, but so is 'default' ;-) > + break; > + } > + > + /* Now, optionally override the presets. */ > + > + switch (kasan_arg_stacktrace) { > + case KASAN_ARG_STACKTRACE_OFF: > + static_branch_disable(&kasan_flag_stacktrace); > + break; > + case KASAN_ARG_STACKTRACE_ON: > + static_branch_enable(&kasan_flag_stacktrace); > + break; > + default: > + break; > + } > + > + switch (kasan_arg_fault) { > + case KASAN_ARG_FAULT_REPORT: > + kasan_flag_panic = false; > + break; > + case KASAN_ARG_FAULT_PANIC: > + kasan_flag_panic = true; > + break; > + default: > + break; > + } > + Would be good to get rid of the 'default' cases here. > pr_info("KernelAddressSanitizer initialized\n"); > } > > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h > index 2d3c99125996..5eff3d9f624e 100644 > --- a/mm/kasan/kasan.h > +++ b/mm/kasan/kasan.h > @@ -5,6 +5,22 @@ > #include > #include > > +#ifdef CONFIG_KASAN_HW_TAGS > +#include > +DECLARE_STATIC_KEY_FALSE(kasan_flag_stacktrace); > +static inline bool kasan_stack_collection_enabled(void) > +{ > + return static_branch_unlikely(&kasan_flag_stacktrace); > +} > +#else > +static inline bool kasan_stack_collection_enabled(void) > +{ > + return true; > +} > +#endif > + > +extern bool kasan_flag_panic __ro_after_init; > + > #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS) > #define KASAN_GRANULE_SIZE (1UL << KASAN_SHADOW_SCALE_SHIFT) > #else > diff --git a/mm/kasan/report.c b/mm/kasan/report.c > index 25ca66c99e48..7d86af340148 100644 > --- a/mm/kasan/report.c > +++ b/mm/kasan/report.c > @@ -99,6 +99,10 @@ static void end_report(unsigned long *flags) > panic_on_warn = 0; > panic("panic_on_warn set ...\n"); > } > +#ifdef CONFIG_KASAN_HW_TAGS > + if (kasan_flag_panic) > + panic("kasan.fault=panic set ...\n"); > +#endif > kasan_enable_current(); > } > > @@ -161,8 +165,8 @@ static void describe_object_addr(struct kmem_cache *cache, void *object, > (void *)(object_addr + cache->object_size)); > } > > -static void describe_object(struct kmem_cache *cache, void *object, > - const void *addr, u8 tag) > +static void describe_object_stacks(struct kmem_cache *cache, void *object, > + const void *addr, u8 tag) > { > struct kasan_alloc_meta *alloc_meta = kasan_get_alloc_meta(cache, object); > > @@ -190,7 +194,13 @@ static void describe_object(struct kmem_cache *cache, void *object, > } > #endif > } > +} > > +static void describe_object(struct kmem_cache *cache, void *object, > + const void *addr, u8 tag) > +{ > + if (kasan_stack_collection_enabled()) > + describe_object_stacks(cache, object, addr, tag); > describe_object_addr(cache, object, addr); > }