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=-11.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 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 EF7CFC388F9 for ; Wed, 11 Nov 2020 18:30:23 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4CF42206FB for ; Wed, 11 Nov 2020 18:30:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Lo/D0Q7/"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=google.com header.i=@google.com header.b="eUBuV6ws" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4CF42206FB Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=SD7e9JH8jSf81TZ6gJqpuPNCq6kog/DCLF1kDS8BDow=; b=Lo/D0Q7/r7OuRRlZnYG9K4tEM pVAU/maOloXGaxOIHJxHE/3OWwD2FbvqC5jk7lHvh/+rUbxwqtNcT1S8GBqB8Ppza9FCRpNB74VX1 M2cU5My2ciTi1Uxv5j/r64YHJNzJEyL7mSWjpCetoVPzOekPYLz+kVHGC4KHv7duNQfJ7OkFFf1ba cK4ilahy2Kbf4Cyw+Ov6lTsq6zBQ4YZYP2Vzoq6+8hx2pl6mIc7xH7+3e9MaCTRfvJR2irM6Z8KVq sZPKq7j1iPsHY+xsJwuaRlo5Fa0GlGVpmhBkdQMpzNHY9TkL9E8XfiX9cp72D7ICJwn+EoiG9/61H xy2mAFLBQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kcusI-00007c-Li; Wed, 11 Nov 2020 18:29:42 +0000 Received: from mail-wr1-x443.google.com ([2a00:1450:4864:20::443]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kcusF-00007F-Mq for linux-arm-kernel@lists.infradead.org; Wed, 11 Nov 2020 18:29:41 +0000 Received: by mail-wr1-x443.google.com with SMTP id s8so3459984wrw.10 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=UwQbqP4U4QbBnbrwrMX9FyTlAlZtx8KyNJioOvZ6hQZq9BZ/N5HMJn5fALuM0AsRDT LThANjHvNWwc+s4TdGBb5fqs30VATDxQmObIcsVol+RLRJzaBv7vs1/NVMW5V4A8ff3X E4KkQYiBKlmV5uNQcLaPY8I1mYLCcgRqoUXDR9gmlNSTalRl8ZbGoltBCqxokQuNkhf+ 9LVG4RidOKDrbNKf21mbCSCGY8MI9dVb54YNBiyJ7mETQSy2l9PYLhkUQlOzpb1H0acP 8FDnSEmB6uMKAq5nxKXjRK+QEvsglpyZZvZYre/Dr0db1Nr8vbk0iJQHA25KKWv24uLD G9Rw== X-Gm-Message-State: AOAM532wZod1Dc8O4UgbECInysHwfDe1efmAIcvgF9OrYZ+z5+G1adAT VTcc97Y3aZuvh4A5l8g6wWyxyQ== 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 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-Disposition: inline In-Reply-To: User-Agent: Mutt/1.14.6 (2020-07-11) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201111_132939_825747_D6BF4DFF X-CRM114-Status: GOOD ( 42.98 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-kernel@lists.infradead.org, Branislav Rankov , Catalin Marinas , Kevin Brodsky , Will Deacon , linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, linux-mm@kvack.org, Alexander Potapenko , Dmitry Vyukov , Andrey Ryabinin , Andrew Morton , Vincenzo Frascino , Evgenii Stepanov Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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); > } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel