From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42d.google.com (mail-pf1-x42d.google.com [IPv6:2607:f8b0:4864:20::42d]) by sourceware.org (Postfix) with ESMTPS id 461F33858C39 for ; Mon, 17 Jul 2023 23:40:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 461F33858C39 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-x42d.google.com with SMTP id d2e1a72fcca58-66c729f5618so5202631b3a.1 for ; Mon, 17 Jul 2023 16:40:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1689637214; x=1692229214; 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=PtzElzZJP3sASG/JY/hBDf5P4Q9nTjfn3AwPTGKm+p8=; b=QNcwH4R8b/W/CSP8hmrekL7HbqfKY47RYcNOVeNQF+g3HhsPsymVsga4VJ8dr1K/lF dpHHOhppUFK0iWChT7hlzFnKZCKpJA3Dh3Zlr+fVneCPV12+xR7ZkohZAUfi/Y5smXIh 2dXYAked308xSFdUwAhv+c+grb6JoZvVcb/xg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689637214; x=1692229214; 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=PtzElzZJP3sASG/JY/hBDf5P4Q9nTjfn3AwPTGKm+p8=; b=GxsCR6BOoyafGThJpAYlKV8fQ+eYr4Xq80htpoU45ARlIQihYNS/skslxCwI+WVXCY xdm+4XGG/cFODmqkHBB0kP7RlofqMYUgUVn/ZcpTgz3clzCuI+IsOXCHwbL20I2LDS4r 6WF+rjJqhifJbWNQDkGrXoWvkFdUfCJ3QteuDtoMXieIuismDRUkA3mrEzn8vya1lX9A rp8MHjRLzMkMF3qk8nOgSazZk9IPVo715v1NYG4MXEo+a1cUzO39wxENRJhqJuylswaP RF3GwfNAol+bXiENBD1fpD/7tIExueLIHsPN4MaUdQ/1iG5QVqWBVobWgiwbZeem9eKD S98g== X-Gm-Message-State: ABy/qLYbPsX6gqX6Mez2rwxGa+zCYiaDjuMs8ACgdasye9J8YB5t0OCP lxg+nEQtpnZqS0ptnWPYV+nApw== X-Google-Smtp-Source: APBJJlF5qXDz748iU4dvy93Ypl4qheYfy0fMZvKZlSjahdGzEFy+DI39+yUTBmlJJ+zBzmda629asQ== X-Received: by 2002:a05:6a00:194a:b0:676:ad06:29d7 with SMTP id s10-20020a056a00194a00b00676ad0629d7mr18097632pfk.15.1689637213411; Mon, 17 Jul 2023 16:40:13 -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 f24-20020aa78b18000000b006815fbe3240sm345847pfd.11.2023.07.17.16.40.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Jul 2023 16:40:12 -0700 (PDT) Date: Mon, 17 Jul 2023 16:40:12 -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: <202307171612.406D82C39@keescook> References: <20230525161450.3704901-1-qing.zhao@oracle.com> <202305261218.2420AB8E0@keescook> <202307131311.1F30C4357@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 Mon, Jul 17, 2023 at 09:17:48PM +0000, Qing Zhao wrote: > > > On Jul 13, 2023, at 4:31 PM, Kees Cook wrote: > > > > In the bug, the problem is that "p" isn't known to be allocated, if I'm > > reading that correctly? > > I think that the major point in PR109557 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557): > > for the following pointer p.3_1, > > p.3_1 = p; > _2 = __builtin_object_size (p.3_1, 0); > > Question: why the size of p.3_1 cannot use the TYPE_SIZE of the pointee of p when the TYPE_SIZE can be determined at compile time? > > Answer: From just knowing the type of the pointee of p, the compiler cannot determine the size of the object. Why is that? "p" points to "struct P", which has a fixed size. There must be an assumption somewhere that a pointer is allocated, otherwise __bos would almost never work? > Therefore the bug has been closed. > > In your following testing 5: > > > 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 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. > > Since the call to the routine “alloc_annotated" cannot be inlined, GCC does not see any allocation calls for the pointer p. Correct. > At the same time, due to the same reason as PR109986, GCC cannot determine the size of the object by just knowing the TYPE_SIZE > of the pointee of p. So the difference between test 3 and test 5 is that "p" is explicitly dereferenced to find "array", and therefore an assumption is established that "p" is allocated? > So, this is exactly the same issue as PR109557. It’s an existing behavior per the current __buitlin_object_size algorithm. > I am still not very sure whether the situation in PR109557 can be improved or not, but either way, it’s a separate issue. Okay, so the issue is one of object allocation visibility (or assumptions there in)? > Please see the new testing case I added for PR109557, you will see that the above case 5 is a similar case as the new testing case in PR109557. I will ponder this a bit more to see if I can come up with a useful test case to replace the part from "test 5" above. > > > > 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.) > > The counted_by attribute is used to annotate a Flexible array member on how many elements it will have. > However, if this information can not accurately reflect the real number of elements for the array allocated, > What’s the purpose of such information? For example, imagine code that allocates space for 100 elements since the common case is that the number of elements will grow over time. Elements are added as it goes. For example: struct grows { int alloc_count; int valid_count; struct element item[] __counted_by(valid_count); } *p; void something(void) { p = malloc(sizeof(*p) + sizeof(*p->item) * 100); p->alloc_count = 100; p->valid_count = 0; /* this loop doesn't check that we don't go over 100. */ while (items_to_copy) { struct element *item_ptr = get_next_item(); /* __counted_by stays in sync: */ p->valid_count++; p->item[p->valid_count - 1] = *item_ptr; } } We would want to catch cases there p->item[] is accessed with an index that is >= p->valid_count, even though the allocation is (currently) larger. However, if we ever reached valid_count >= alloc_count, we need to trap too, since we can still "see" the true allocation size. Now, the __alloc_size hint is visible in very few places, so if there is a strong reason to do so, I can live with saying that __counted_by takes full precedence over __alloc_size. It seems it should be possible to compare when both are present, but I can live with __counted_by being the universal truth. :) > > >> 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. > > What’s your mean by “only the least of the two is used”? I was just restating the above: if size information is available via both __alloc_size and __counted_by, the smaller of the two should be used. > > > This may be > > desirable in a few situations. One example would be a large allocation > > that is slowly filled up by the program. > > So, for such situation, whenever the allocation is filled up, the field that hold the “counted_by” attribute should be increased at the same time, > Then, the “counted_by” value always sync with the real allocation. > > I.e. the counted_by member is > > slowly increased during runtime (but not beyond the true allocation size). > > Then there should be source code to increase the “counted_by” field whenever the allocated space increased too. > > > > 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. > > The point is: allocation size should synced with the value of “counted_by”. LLVM’s RFC also have the similar requirement: > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18 Right, I'm saying it would be nice if __alloc_size was checked as well, in the sense that if it is available, it knows without question what the size of the allocation is. If __alloc_size and __counted_by conflict, the smaller of the two should be the truth. But, as I said, if there is some need to explicitly ignore __alloc_size when __counted_by is present, I can live with it; we just need to document it. If the RFC and you agree that the __counted_by variable can only ever be (re)assigned after the flex array has been (re)allocated, then I guess we'll see how it goes. :) I think most places in the kernel using __counted_by will be fine, but I suspect we may have cases where we need to update it like in the loop I described above. If that's true, we can revisit the requirement then. :) -Kees -- Kees Cook