From patchwork Fri Sep 2 20:43:49 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 12964645 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7B016C6FA83 for ; Fri, 2 Sep 2022 20:44:01 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229975AbiIBUoA (ORCPT ); Fri, 2 Sep 2022 16:44:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56504 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230144AbiIBUn6 (ORCPT ); Fri, 2 Sep 2022 16:43:58 -0400 Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E9263FC316 for ; Fri, 2 Sep 2022 13:43:56 -0700 (PDT) Received: by mail-pl1-x632.google.com with SMTP id v5so2954995plo.9 for ; Fri, 02 Sep 2022 13:43:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date; bh=nXV/kYcxRGP4+PC/wC/E1WmMIMB/DEMUyjUxzaTreGw=; b=cHlmGDucWugffybtU1NXRpK84RfhetNHSyxRNpz4OPVFMNCPebRN6HUazGQvm8DxjM Ca5mMNM8lTvPGRnENwR8gECD6NiKDnXKaP2gbjBA6Inj1r56/1vaqNqDUIMbaxigFFBO AqXXJDP7flKezJmMXtpBKDn54dIz26/0Tg1ck= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date; bh=nXV/kYcxRGP4+PC/wC/E1WmMIMB/DEMUyjUxzaTreGw=; b=iM9jDuMbTZhTmV+lkD2Ujaqz4eAW/tAxz1Addyc/ZWYsN3e0N23e99jkMaP5ZEZDpq 4iAaR7uVgMBMZ0nPNR0Kvg9JDazFjDtBt0B77XUXoxI7+u2KSWVmgf+s9Ip1uHy4sQtq x3ruBl7C+kwdq2Emi53/epnqvzmok1TX8Q/aT+wVyHxRX3rVwRECcCFQB7+tGaPq9/ig QSqgL8cT9KfOoiYgAA5AmAnelS1y1czPEfviuMzCkvBga3E/SprGKaWjtKrmX7DGOpYi cZno2FwRgKGTkvgOovemDug6Xcz1sGl5pE3/aCcWbE1o/YeVFx9DQGCqlN1B9gZhtRUV htDg== X-Gm-Message-State: ACgBeo2RKp3A8vT7dCFPi8hHHAXeiBweq7p34s5TX1dTMtTub9Ndj4iP UZTomE21Zjtm1vHNav+oKYulYA== X-Google-Smtp-Source: AA6agR4OTKbaTPmi+l9Osgr3f3/iWEvZcmM5T5mq8YCP3H+BKdqRtF3oXu9Ht4jAVa9+ZFw4qzTJiA== X-Received: by 2002:a17:903:32cd:b0:16f:c31:7034 with SMTP id i13-20020a17090332cd00b0016f0c317034mr36867541plr.126.1662151436441; Fri, 02 Sep 2022 13:43:56 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id p5-20020aa79e85000000b0053826eaa1c7sm2242366pfq.22.2022.09.02.13.43.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Sep 2022 13:43:53 -0700 (PDT) From: Kees Cook To: Nick Desaulniers Cc: Kees Cook , Nathan Chancellor , Tom Rix , Andrew Morton , Vlastimil Babka , "Steven Rostedt (Google)" , David Gow , Yury Norov , Masami Hiramatsu , Sander Vanheule , linux-hardening@vger.kernel.org, llvm@lists.linux.dev, Peter Zijlstra , Josh Poimboeuf , Dan Williams , Isabella Basso , Eric Dumazet , Rasmus Villemoes , Eric Biggers , Hannes Reinecke , linux-kernel@vger.kernel.org Subject: [PATCH v2 1/3] fortify: Fix __compiletime_strlen() under UBSAN_BOUNDS_LOCAL Date: Fri, 2 Sep 2022 13:43:49 -0700 Message-Id: <20220902204351.2521805-2-keescook@chromium.org> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20220902204351.2521805-1-keescook@chromium.org> References: <20220902204351.2521805-1-keescook@chromium.org> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=3151; h=from:subject; bh=DcSHHHF6kn1GliVVBN0Pagg+IpCny/Ui+zrhTxFh/Ig=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBjEmsGk86XdtCYEmOQCgPx8egB0fdSS3e9m2VZTI23 uTv0O9uJAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCYxJrBgAKCRCJcvTf3G3AJuYcD/ 46zM6qv3smVwB5rZ2yT5KmQ/6rKo6NAA9bRqTf+cVhi63JuonH9xJUJIGZG+MoTCKjisfYwnMSQzxC JzaNXj7WkEtgdnD0FwG7abuEJKIfgtU0XaT1fFJE99rSCXoGFWzhdvyDwLnhbPqN5xM7bKjP6fs8/i HA4rfsFA2io/zAKOKbS0LcUohuLvzpmhh1FooGaDJNFpmkJEp5NIInreEiryFnGjvFV/9u8ICW8qX0 jQRNsTRh/oUF/wY0O25PWfyGOHq+pdb2z54/j+NOAWzjvSC09LbnOygH+6S692/psVAj5EggYDO/jF f8Lnz+jjlp/EVUqaYqsolrBOll6bfzSPd0c2Ot/oNu0XcyCyDA88YD64pWvgJCl/kXs3Vi05qm2hKW rfGR2CUtas8Cde1LUKAq9J7g4RNLjvGpZFXPMdB2K2uNw3BrNl0AmxaxJuT96wAAi2Ugv6MuSu8It7 Pbll63lCPLIlUSq9WOFBnKwgmbE0EqA3Z+f7s2i68q+8cLdESF7EDzMJYLkY+oKbrUin5IwJsrNMx5 aYmXCyvApdqpJQxT0ApO0GH7YxI0gTCLPb62iCjDvlsyb4ekhTCgPwxydRAx6fujxccIrOhnhVCAMV +WTnx3IlX62a2AP+WQ7hZQiKnc4G2GYlnWz1ydiMrJAX0gNyARjrGALryXSA== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Precedence: bulk List-ID: X-Mailing-List: linux-hardening@vger.kernel.org With CONFIG_FORTIFY=y and CONFIG_UBSAN_LOCAL_BOUNDS=y enabled, we observe a runtime panic while running Android's Compatibility Test Suite's (CTS) android.hardware.input.cts.tests. This is stemming from a strlen() call in hidinput_allocate(). __compiletime_strlen() is implemented in terms of __builtin_object_size(), then does an array access to check for NUL-termination. A quirk of __builtin_object_size() is that for strings whose values are runtime dependent, __builtin_object_size(str, 1 or 0) returns the maximum size of possible values when those sizes are determinable at compile time. Example: static const char *v = "FOO BAR"; static const char *y = "FOO BA"; unsigned long x (int z) { // Returns 8, which is: // max(__builtin_object_size(v, 1), __builtin_object_size(y, 1)) return __builtin_object_size(z ? v : y, 1); } So when FORTIFY_SOURCE is enabled, the current implementation of __compiletime_strlen() will try to access beyond the end of y at runtime using the size of v. Mixed with UBSAN_LOCAL_BOUNDS we get a fault. hidinput_allocate() has a local C string whose value is control flow dependent on a switch statement, so __builtin_object_size(str, 1) evaluates to the maximum string length, making all other cases fault on the last character check. hidinput_allocate() could be cleaned up to avoid runtime calls to strlen() since the local variable can only have literal values, so there's no benefit to trying to fortify the strlen call site there. Perform a __builtin_constant_p() check against index 0 earlier in the macro to filter out the control-flow-dependant case. Add a KUnit test for checking the expected behavioral characteristics of FORTIFY_SOURCE internals. Cc: Nathan Chancellor Cc: Tom Rix Cc: Andrew Morton Cc: Vlastimil Babka Cc: "Steven Rostedt (Google)" Cc: David Gow Cc: Yury Norov Cc: Masami Hiramatsu Cc: Sander Vanheule Cc: linux-hardening@vger.kernel.org Cc: llvm@lists.linux.dev Co-developed-by: Nick Desaulniers Signed-off-by: Nick Desaulniers Signed-off-by: Kees Cook Reviewed-by: Nick Desaulniers Tested-by: Android Treehugger Robot --- include/linux/fortify-string.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index eed2119b23c5..07d5d1921eff 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -19,7 +19,8 @@ void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning(" unsigned char *__p = (unsigned char *)(p); \ size_t __ret = (size_t)-1; \ size_t __p_size = __builtin_object_size(p, 1); \ - if (__p_size != (size_t)-1) { \ + if (__p_size != (size_t)-1 && \ + __builtin_constant_p(*__p)) { \ size_t __p_len = __p_size - 1; \ if (__builtin_constant_p(__p[__p_len]) && \ __p[__p_len] == '\0') \