From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) by sourceware.org (Postfix) with ESMTPS id 93B373858D1E for ; Thu, 13 Jul 2023 20:31:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 93B373858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=chromium.org Received: by mail-pf1-x433.google.com with SMTP id d2e1a72fcca58-666eec46206so1140062b3a.3 for ; Thu, 13 Jul 2023 13:31:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1689280288; x=1691872288; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=3qE7fXn6JbYqSCafyG9j8xnMUXIVTT5fgAbrx2h6M2A=; b=PbLTMFgA1L50iwlWdVKvoYLX+33iEquVe6RsXabknBeDSwDOnt0l8YcLd46IyxQ34B gR7GUdDEFUdriH6EPkjPyQuB6IU+/Uv/iZ+fWBgDdOAhZcmA3Qjqeg7l+xB+MHQnf+FE KqW+KClJtakLKKo03kq8YrPtDHfVBXGAcnmB0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689280288; x=1691872288; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3qE7fXn6JbYqSCafyG9j8xnMUXIVTT5fgAbrx2h6M2A=; b=jGyVf9VQqwTHctzDSXE20puUlIjtG6WF/flcbh0TgUqDyQLKvFqTBSvsWsFPB3mjUP g+wNS7gjI82pnaeVu2wmjvdejQQ9QiQ8x+NlOl42vOxE3i4glQ4h+oPBK8qGEgnC0Tnt SG3puh9k1XNE0+aJ8xCKUt2NTrPLjvfoxxo6HFf/SB1qC+BF3HnKrx9HKZp/eDUrrf0+ rq9vdye9hiWvl/WBZQ6OWp2B8dW7POwy5GH1lmnFsPZ+RFZYCcHYNQl0wr0pDpZCCLZe jmT7fgF63FqdhvG+qYjpFpnLkeWrpgzLAXaggQtK/kPn1Mlrt9Yl5A6qbKAQxcLuXHdS qvNA== X-Gm-Message-State: ABy/qLbW5FTeBBWfdOFBM6SxyXNiGbag/TI1OCawypWSdc+qUMThfhBM dE5iHOB34ptng64ea2TEQbCFQw== X-Google-Smtp-Source: APBJJlG7LXSShbMmp63CtNAmTbrYpnxo/wAlSe0Ae/X+KYo7ulhxKoSaGAepjAKaShdzzJgq7bf1Gw== X-Received: by 2002:a05:6a20:841c:b0:130:6b27:729f with SMTP id c28-20020a056a20841c00b001306b27729fmr3600822pzd.3.1689280288435; Thu, 13 Jul 2023 13:31:28 -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 y17-20020aa78551000000b00682562bf479sm5858407pfn.53.2023.07.13.13.31.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 13 Jul 2023 13:31:27 -0700 (PDT) Date: Thu, 13 Jul 2023 13:31:26 -0700 From: Kees Cook To: Qing Zhao Cc: "joseph@codesourcery.com" , "richard.guenther@gmail.com" , "jakub@redhat.com" , "gcc-patches@gcc.gnu.org" , "siddhesh@gotplt.org" , "uecker@tugraz.at" , "isanbard@gmail.com" Subject: Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896) Message-ID: <202307131311.1F30C4357@keescook> References: <20230525161450.3704901-1-qing.zhao@oracle.com> <202305261218.2420AB8E0@keescook> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Jul 06, 2023 at 06:56:21PM +0000, Qing Zhao wrote: > Hi, Kees, > > I have updated my V1 patch with the following changes: > A. changed the name to "counted_by" > B. changed the argument from a string to an identifier > C. updated the documentation and testing cases accordingly. Sounds great! > > And then used this new gcc to test https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c (with the following change) > [opc@qinzhao-ol8u3-x86 Kees]$ !1091 > diff array-bounds.c array-bounds.c.org > 32c32 > < # define __counted_by(member) __attribute__((counted_by (member))) > --- > > # define __counted_by(member) __attribute__((__element_count__(#member))) > 34c34 > < # define __counted_by(member) __attribute__((counted_by (member))) > --- > > # define __counted_by(member) /* __attribute__((__element_count__(#member))) */ > > Then I got the following result: > [opc@qinzhao-ol8u3-x86 Kees]$ ./array-bounds 2>&1 | grep -v ^'#' > TAP version 13 > 1..12 > ok 1 global.fixed_size_seen_by_bdos > ok 2 global.fixed_size_enforced_by_sanitizer > not ok 3 global.unknown_size_unknown_to_bdos > not ok 4 global.unknown_size_ignored_by_sanitizer > ok 5 global.alloc_size_seen_by_bdos > ok 6 global.alloc_size_enforced_by_sanitizer > not ok 7 global.element_count_seen_by_bdos > ok 8 global.element_count_enforced_by_sanitizer > not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos > not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer > ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos > ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer > > The same as your previous results. Then I took a look at all the failed testing: 3, 4, 7, 9, and 10. And studied the reasons for all of them. > > in a summary, there are two major issues: > 1. The reason for the failed testing 7 is the same issue as I observed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557 > Which is not a bug, it’s an expected behavior. In the bug, the problem is that "p" isn't known to be allocated, if I'm reading that correctly? I'm not sure this is a reasonable behavior, but let me get into the specific test, which looks like this: TEST(counted_by_seen_by_bdos) { struct annotated *p; int index = MAX_INDEX + unconst; p = alloc_annotated(index); REPORT_SIZE(p->array); /* 1 */ EXPECT_EQ(sizeof(*p), offsetof(typeof(*p), array)); /* Check array size alone. */ /* 2 */ EXPECT_EQ(__builtin_object_size(p->array, 1), SIZE_MAX); /* 3 */ EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1), p->foo * sizeof(*p->array)); /* Check check entire object size. */ /* 4 */ EXPECT_EQ(__builtin_object_size(p, 1), SIZE_MAX); /* 5 */ EXPECT_EQ(__builtin_dynamic_object_size(p, 1), sizeof(*p) + p->foo * sizeof(*p->array)); } Test 1 trivially passes -- this is just a sanity check. Test 2 should be SIZE_MAX because __bos only knows compile-time sizes. Test 3 should pass: counted_by knows the allocation size and can be queried as part of the "p" object. Test 4 should be SIZE_MAX because __bos only knows compile-time sizes. Test 5 should pass as well, since, again, p can be examined. Passing p to __bdos implies it is allocated and the __counted_by annotation can be examined. If "return p->array[index];" would be caught by the sanitizer, then it follows that __builtin_dynamic_object_size(p, 1) must also know the size. i.e. both must examine "p" and its trailing flexible array and __counted_by annotation. > > 2. The common issue for the failed testing 3, 4, 9, 10 is: > > for the following annotated structure: > > ==== > struct annotated { > unsigned long flags; > size_t foo; > int array[] __attribute__((counted_by (foo))); > }; > > > struct annotated *p; > int index = 16; > > p = malloc(sizeof(*p) + index * sizeof(*p->array)); // allocated real size > > p->foo = index + 2; // p->foo was set by a different value than the real size of p->array as in test 9 and 10 Right, tests 9 and 10 are checking that the _smallest_ possible value of the array is used. (There are two sources of information: the allocation size and the size calculated by counted_by. The smaller of the two should be used when both are available.) > or > p->foo was not set to any value as in test 3 and 4 For tests 3 and 4, yes, this was my mistake. I have fixed up the tests now. Bill noticed the same issue. Sorry for the think-o! > ==== > > i.e, the value of p->foo is NOT synced with the number of elements allocated for the array p->array. > > I think that this should be considered as an user error, and the documentation of the attribute should include > this requirement. (In the LLVM’s RFC, such requirement was included in the programing model: > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18) > > We can add a new warning option -Wcounted-by to report such user error if needed. > > What’s your opinion on this? I think it is correct to allow mismatch between allocation and counted_by as long as only the least of the two is used. This may be desirable in a few situations. One example would be a large allocation that is slowly filled up by the program. I.e. the counted_by member is slowly increased during runtime (but not beyond the true allocation size). Of course allocation size is only available in limited situations, so the loss of that info is fine: we have counted_by for everything else. -- Kees Cook