Message ID | 20230922175224.work.712-kees@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp5814134vqi; Fri, 22 Sep 2023 12:20:00 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEPKvor59uy6eyioaeiAmL6qnBTGHevVW1EQiiEqSJx9oWgFxIk1rfHl99blISRWvDw5Wqb X-Received: by 2002:a05:6358:71c2:b0:13a:6748:9312 with SMTP id u2-20020a05635871c200b0013a67489312mr578336rwu.19.1695410400686; Fri, 22 Sep 2023 12:20:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695410400; cv=none; d=google.com; s=arc-20160816; b=WMec5ZvdvMLdJiE80qsHOrQmB+nT4t1QFmC54YrgHMZgw5rVM9aToE+szV3Yusa8qg RpPs6AYEEja2zxcgeLzpTLH0PIRsN9a/Da9mY56D04zRfBGq08cHWqjl0+pUJ6TMbqyW YSniY3hM4OzCHLvDw8ksGIrYA8Jyd3F+p9ox44uDoUkVGfMLbcL3nvT/spGbOEIyXFVB YAhvaWWu+Z2dUpLtFMMuX3pBB2ouu6x3FTDOXjXuDtk0uEoeuc7vjdb8aXE1SPdoVFJ3 FpSYz1Ph+fH+0nqc36ZW4WJKG1li+a+mi0mvaX/SIz9of2qN2v5VNa98efW5A09hp/kQ dUQA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=0V3jWlF6ThW9BuslQEoTcg1mHcaUXfGXFyJTs8W3R6E=; fh=KwSGAiZOyyHfQcuDHX9CN2zaV4KFJaZIZ16ft6+Xk2k=; b=caXNHuQ8zDN4V3b2CFUJ46ArI2U4M143oz98zSTQ1Q9hseXJW+941snFs8HH181Btg ZJ54NOAvOH3LD+IpVaoQu2npV7Q3KYTfGlafu0x8lYZJjKHswx4Y4EX7h6VypFaJyCQO 3eEGpHUNsVW3Db30Wom7VhfBLf4p5ljtc/ZswTbZy9xtGGloE3Dw6qfYnmVrwbt4vtX8 uZvkB2PqQEQ58diOv9ZA9dK4aDskIk4uMFLJawAVhmqD5016/6u9u8BHKNgDjU9wsapW krDJt9oO3qYq7hFT7ncnlQ2Vp1C9NiNsgo24blfsSjFfaIOOZ15gws81IHMHb3vI0PT2 SHgw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=WNsNx3lE; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id be3-20020a656e43000000b0057e5d66c200si797359pgb.462.2023.09.22.12.20.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Sep 2023 12:20:00 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=WNsNx3lE; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 3E3A6839E0FF; Fri, 22 Sep 2023 10:54:45 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233825AbjIVRyQ (ORCPT <rfc822;pwkd43@gmail.com> + 28 others); Fri, 22 Sep 2023 13:54:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46098 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233653AbjIVRxr (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 22 Sep 2023 13:53:47 -0400 Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA3281980 for <linux-kernel@vger.kernel.org>; Fri, 22 Sep 2023 10:52:26 -0700 (PDT) Received: by mail-pf1-x434.google.com with SMTP id d2e1a72fcca58-690bd59322dso2022674b3a.3 for <linux-kernel@vger.kernel.org>; Fri, 22 Sep 2023 10:52:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1695405146; x=1696009946; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=0V3jWlF6ThW9BuslQEoTcg1mHcaUXfGXFyJTs8W3R6E=; b=WNsNx3lEa3pYifcOXRTBiyOirDBQYr/iVR8Oz8syKOitmjmQguKVvBDEf8giSm1okU CvYIJQnq6juXYJKyCgUdy4fkPjQOzJMUwzQnHEuelSaAw0khzW0Z/A0wqgVzGNC+FGZN UzfOcX+C8XRPS77EiW7GLkamq/G3i3EKwnmD8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695405146; x=1696009946; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=0V3jWlF6ThW9BuslQEoTcg1mHcaUXfGXFyJTs8W3R6E=; b=mQ2JZJ3yBG8VDwRk1lXPB0/bQfy0XpJSb9S9joekHFS9nD4y1Ra6M/42+Ul5ekh2Ie FkOBhJFSLxfu8TRudUu23dlt9C9Ut2jwNpi/Zo0jiTTSuNW/prygCNE553BD3iMKYI2t ogbXxhS+ZAPvQm0m6v/retBY4hk4zT/eWd/C74MEIatccVpzRIp9fMql10pi5uePGpQV zvx5TgVGNdSyvOhv/QOsn915r+e2fEnAazq04Fj8jChyvMSbJN+kkV9YQhWxl0j3N/6A JkeDArfQUmIbIRVE+B1oYDHMPLbnhsTmhJIPGL0etG2UYltYaoc20KTgfLp/SxNx1rpY /yQg== X-Gm-Message-State: AOJu0YzTUFS3KG4xehke/GWib4tmG5eikXIlMSoRMMt57PqywM0JORSC fBCRYktouZ3ykls/VHuH9jjVdA== X-Received: by 2002:a05:6a20:918a:b0:149:9b2f:a79d with SMTP id v10-20020a056a20918a00b001499b2fa79dmr290578pzd.6.1695405146334; Fri, 22 Sep 2023 10:52:26 -0700 (PDT) Received: from www.outflux.net (198-0-35-241-static.hfc.comcastbusiness.net. [198.0.35.241]) by smtp.gmail.com with ESMTPSA id n6-20020a17090ade8600b002680dfd368dsm3539391pjv.51.2023.09.22.10.52.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Sep 2023 10:52:25 -0700 (PDT) From: Kees Cook <keescook@chromium.org> To: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org>, kexec@lists.infradead.org, Baoquan He <bhe@redhat.com>, Vivek Goyal <vgoyal@redhat.com>, Dave Young <dyoung@redhat.com>, Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers <ndesaulniers@google.com>, Tom Rix <trix@redhat.com>, linux-kernel@vger.kernel.org, llvm@lists.linux.dev, linux-hardening@vger.kernel.org Subject: [PATCH] kexec: Annotate struct crash_mem with __counted_by Date: Fri, 22 Sep 2023 10:52:24 -0700 Message-Id: <20230922175224.work.712-kees@kernel.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=1201; i=keescook@chromium.org; h=from:subject:message-id; bh=L7QtPm9sV53utHLzzgBlBEkBDQ4wLhHXIn/rOvcMRB8=; b=owEBbQKS/ZANAwAKAYly9N/cbcAmAcsmYgBlDdRYVSnSoLrnQY5IDtf2GC3u6QkTIyJo8C8QH UxbqYg6Z1eJAjMEAAEKAB0WIQSlw/aPIp3WD3I+bhOJcvTf3G3AJgUCZQ3UWAAKCRCJcvTf3G3A JiO2D/4/umN2M56qMMDxw25gfk0XXrNasJyvVidbf0iVWd2FgMZqMzVjLLRZKgALKtPtlbDlkh2 uMauyeoWwoJreiokLRDV03uy+dbxUuXicnyG1bqS0hQX79TIbLta7QucwkDPvl6ApHxx5G7/lM5 n1+Vo5Onde7/uK/2hXqgKAZObP3bWuBXqt/r7JbPJp709MTsA0KotCN0R3ZCUfDz2t8xfPIxvpR S/BBPo6ZplYLWO9EVXoJzCvwZxEGL0JmSI8IMvIYrw6hZRXg1j/dR/pmBplYZnXwLByQXsphPpR S1nRV9zZoQGb6SUX4mf/M71DVfcZKchFA9BihrGb4YXnLb47CU6/+m0AXiLZp5Qx5eVC0cZTHgM Qps2mjfP+ph6xuFEcF/gF8bWWrNbKVvtftK30Li+TojYHIK2xvrdmuBbOCY6g6EVijg0KCGHx8K i9yJ+ibNvqF2ArwEQHi13ri0WBKLiujrfoInUBfMHwjtOxZqjnVpwz1I4W5F22E6+6rWL1CaSZA QOK0C9uuoYEZJkZ74kv04YXgsLbr85durHrVptzD6/83FJLlgZAwX38fwmJ54utqv5Vmvv7EPlN eFnyapYuZ/+5urqf5Q8HplGM5D3AFfSZ6y+Y8OToC74qQf4wevVdjZsyKPHA2o6OuQXX0GnGdR8 Eahlr6L R5b0zPWw== X-Developer-Key: i=keescook@chromium.org; a=openpgp; fpr=A5C3F68F229DD60F723E6E138972F4DFDC6DC026 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Fri, 22 Sep 2023 10:54:46 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777766655876794073 X-GMAIL-MSGID: 1777766655876794073 |
Series |
kexec: Annotate struct crash_mem with __counted_by
|
|
Commit Message
Kees Cook
Sept. 22, 2023, 5:52 p.m. UTC
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct crash_mem.
[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/crash_core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 09/22/23 at 10:52am, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct crash_mem. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: kexec@lists.infradead.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > include/linux/crash_core.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h > index 3426f6eef60b..5126a4fecb44 100644 > --- a/include/linux/crash_core.h > +++ b/include/linux/crash_core.h > @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline, > struct crash_mem { > unsigned int max_nr_ranges; > unsigned int nr_ranges; > - struct range ranges[]; > + struct range ranges[] __counted_by(max_nr_ranges); This __counted_by() only makes sense when there's a obvious upper boundary, max_nr_ranges in this case. This heavily depends and isn't much in kernel? E.g struct swap_info_struct->avail_lists[]. Just curious, not related to this patch though. > }; > > extern int crash_exclude_mem_range(struct crash_mem *mem, > -- > 2.34.1 >
On Sat, Sep 23, 2023 at 08:46:47AM +0800, Baoquan He wrote: > On 09/22/23 at 10:52am, Kees Cook wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > > attribute. Flexible array members annotated with __counted_by can have > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > functions). > > > > As found with Coccinelle[1], add __counted_by for struct crash_mem. > > > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > > > Cc: Eric Biederman <ebiederm@xmission.com> > > Cc: kexec@lists.infradead.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > include/linux/crash_core.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h > > index 3426f6eef60b..5126a4fecb44 100644 > > --- a/include/linux/crash_core.h > > +++ b/include/linux/crash_core.h > > @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline, > > struct crash_mem { > > unsigned int max_nr_ranges; > > unsigned int nr_ranges; > > - struct range ranges[]; > > + struct range ranges[] __counted_by(max_nr_ranges); > > This __counted_by() only makes sense when there's a obvious upper > boundary, max_nr_ranges in this case. Yes; it's designed to be the array element count used for the allocation. For example with the above case: nr_ranges += 2; cmem = vzalloc(struct_size(cmem, ranges, nr_ranges)); if (!cmem) return NULL; cmem->max_nr_ranges = nr_ranges; cmem->nr_ranges = 0; nr_ranges is the max count of the elements. _However_, if a structure (like this one) has _two_ counters, one for "in use" and another for "max available", __counted_by could specify the "in use" case, as long as array indexing only happens when that "in use" has been updated. So, if it were: struct crash_mem { unsigned int max_nr_ranges; unsigned int nr_ranges; struct range ranges[] __counted_by(nr_ranges); }; then this would trigger the bounds checking: cmem->ranges[0] = some_range; /* "nr_ranges" is still 0 so index 0 isn't allowed */ cmem->nr_ranges ++; but this would not: cmem->nr_ranges ++; /* index 0 is now available for use. */ cmem->ranges[0] = some_range; > This heavily depends and isn't much in kernel? Which "this" do you mean? The tracking of max allocation is common. Tracking max and "in use" happens in some places (like here), but is less common. > E.g struct swap_info_struct->avail_lists[]. This is even less common: tracking the count externally from the struct, as done there with nr_node_ids. Shakeel asked a very similar question and also pointed out nr_node_ids: https://lore.kernel.org/all/202309221128.6AC35E3@keescook/ > Just curious, not related to this patch though. I'm happy to answer questions! Yeah, as I said in the above thread, I expect to expand what __counted_by can use, and I suspect (hope) a global would be easier to add than an arbitrary expression. :) -Kees
On 09/22/23 at 08:25pm, Kees Cook wrote: > On Sat, Sep 23, 2023 at 08:46:47AM +0800, Baoquan He wrote: > > On 09/22/23 at 10:52am, Kees Cook wrote: > > > Prepare for the coming implementation by GCC and Clang of the __counted_by > > > attribute. Flexible array members annotated with __counted_by can have > > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > > functions). > > > > > > As found with Coccinelle[1], add __counted_by for struct crash_mem. > > > > > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > > > > > Cc: Eric Biederman <ebiederm@xmission.com> > > > Cc: kexec@lists.infradead.org > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > include/linux/crash_core.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h > > > index 3426f6eef60b..5126a4fecb44 100644 > > > --- a/include/linux/crash_core.h > > > +++ b/include/linux/crash_core.h > > > @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline, > > > struct crash_mem { > > > unsigned int max_nr_ranges; > > > unsigned int nr_ranges; > > > - struct range ranges[]; > > > + struct range ranges[] __counted_by(max_nr_ranges); > > > > This __counted_by() only makes sense when there's a obvious upper > > boundary, max_nr_ranges in this case. > > Yes; it's designed to be the array element count used for the > allocation. For example with the above case: > > nr_ranges += 2; > cmem = vzalloc(struct_size(cmem, ranges, nr_ranges)); > if (!cmem) > return NULL; > > cmem->max_nr_ranges = nr_ranges; > cmem->nr_ranges = 0; > > nr_ranges is the max count of the elements. > > _However_, if a structure (like this one) has _two_ counters, one for > "in use" and another for "max available", __counted_by could specify the > "in use" case, as long as array indexing only happens when that "in use" > has been updated. So, if it were: > > struct crash_mem { > unsigned int max_nr_ranges; > unsigned int nr_ranges; > struct range ranges[] __counted_by(nr_ranges); > }; > > then this would trigger the bounds checking: > > cmem->ranges[0] = some_range; /* "nr_ranges" is still 0 so index 0 isn't allowed */ > cmem->nr_ranges ++; > > but this would not: > > cmem->nr_ranges ++; /* index 0 is now available for use. */ > cmem->ranges[0] = some_range; > > > This heavily depends and isn't much in kernel? > > Which "this" do you mean? The tracking of max allocation is common. > Tracking max and "in use" happens in some places (like here), but is > less common. I thought usually it may not have a max counter of the variable length array embeded in struct, seems I was wrong. Here 'this' means the __counted_by() adding for the variable length array. > > > E.g struct swap_info_struct->avail_lists[]. > > This is even less common: tracking the count externally from the struct, > as done there with nr_node_ids. Shakeel asked a very similar question > and also pointed out nr_node_ids: > https://lore.kernel.org/all/202309221128.6AC35E3@keescook/ > > > Just curious, not related to this patch though. > > I'm happy to answer questions! Yeah, as I said in the above thread, > I expect to expand what __counted_by can use, and I suspect (hope) > a global would be easier to add than an arbitrary expression. :) Thanks a lot for these explanation, Kees. LGTM, Acked-by: Baoquan He <bhe@redhat.com>
On Fri, 22 Sep 2023 10:52:24 -0700, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct crash_mem. > > [...] Applied to for-next/hardening, thanks! [1/1] kexec: Annotate struct crash_mem with __counted_by https://git.kernel.org/kees/c/15fcedd43a08 Take care,
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h index 3426f6eef60b..5126a4fecb44 100644 --- a/include/linux/crash_core.h +++ b/include/linux/crash_core.h @@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char *cmdline, struct crash_mem { unsigned int max_nr_ranges; unsigned int nr_ranges; - struct range ranges[]; + struct range ranges[] __counted_by(max_nr_ranges); }; extern int crash_exclude_mem_range(struct crash_mem *mem,