Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-30 Thread Kees Cook
On Fri, Mar 29, 2024 at 04:06:58PM +, Qing Zhao wrote:
> This is the 8th version of the patch.

Thanks for the updated version!

I've done a full Linux kernel build and run through my behavioral
regression test suite. Everything is working as expected.

-Kees

-- 
Kees Cook


Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-29 Thread Tom Tromey
> So, let’s delay the possible support to gdb in a later patch. 

> Does this sound reasonable to you?

It's not really up to me, but sure.  I was just curious if it perhaps
already worked, but not enough to apply the patches and find out.

Tom


Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-29 Thread Tom Tromey
Kees> Does DWARF have such an annotation? Regardless, I think this could be a
Kees> future patch to not hold up landing the initial feature.

Sure, the compiler can emit the array length (and structure size) as a
DWARF expression using the length.

Tom


Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-29 Thread Qing Zhao
Hi,  Tom,

Thanks a lot for the comments. 

It’s good to hear that this new attribute might be able to be used to help gdb. 

We might spend some time to study to use this information in other consumers, 
for example, gdb, in the future, if necessary and possible.  If you have good 
examples to show the importance of using such information in gdb, please let me 
know. I’m glad to study a little more. 

At this time, I agree with Kees, it’s better for the initial patches of the 
“counted-by” support to focus on the the attribute itself and the immediate 
security consumers, such as array bound sanitizer and dynamic object size, etc. 

So, let’s delay the possible support to gdb in a later patch. 

Does this sound reasonable to you?

Qing



> On Mar 29, 2024, at 15:16, Kees Cook  wrote:
> 
> On Fri, Mar 29, 2024 at 12:09:15PM -0600, Tom Tromey wrote:
>>> Qing Zhao  writes:
>> 
>>> This is the 8th version of the patch.
>> 
>>> compare with the 7th version, the difference are:
>> 
>> [...]
>> 
>> Hi.  I was curious to know if the information supplied by this attribute
>> shows up in the DWARF.  It would be good if it did, because that would
>> let gdb correctly print these arrays without user intervention.
> 
> Does DWARF have such an annotation? Regardless, I think this could be a
> future patch to not hold up landing the initial feature.
> 
> -- 
> Kees Cook



Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-29 Thread Kees Cook
On Fri, Mar 29, 2024 at 12:09:15PM -0600, Tom Tromey wrote:
> > Qing Zhao  writes:
> 
> > This is the 8th version of the patch.
> 
> > compare with the 7th version, the difference are:
> 
> [...]
> 
> Hi.  I was curious to know if the information supplied by this attribute
> shows up in the DWARF.  It would be good if it did, because that would
> let gdb correctly print these arrays without user intervention.

Does DWARF have such an annotation? Regardless, I think this could be a
future patch to not hold up landing the initial feature.

-- 
Kees Cook


Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-29 Thread Tom Tromey
> Qing Zhao  writes:

> This is the 8th version of the patch.

> compare with the 7th version, the difference are:

[...]

Hi.  I was curious to know if the information supplied by this attribute
shows up in the DWARF.  It would be good if it did, because that would
let gdb correctly print these arrays without user intervention.

Tom


[PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

2024-03-29 Thread Qing Zhao
Hi,

This is the 8th version of the patch.

compare with the 7th version, the difference are:

updates per Joseph's comments:

1. Wording changes in diagnostics;
   "non flexible" to "non-flexible";
   Diagnostics starts with a lowercase letter;
2. Documentation changes:
   "named ``@var{count}'' to ``@var{count}'';
   use present tense in the documentation;
3. Checking "INTEGRAL_TYPE_P" instead of just INTEGER_TYPE for integer types.
   Add testcases for _Bool/enum/_BitInt count fields. 
4. Add handling for multiple counted_by attributes on the same field:
   Allow duplicates if they name the same field;
   Error when they name different fields.
   Add testcase for this.
5. Updates for comments style.


The 7th version is at:
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648087.html
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648088.html
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648089.html
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648090.html
https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648091.html

It based on the following original proposal:

https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635884.html
Represent the missing dependence for the "counted_by" attribute and its 
consumers

**The summary of the proposal is:

* Add a new internal function ".ACCESS_WITH_SIZE" to carry the size information 
for every reference to a FAM field;
* In C FE, Replace every reference to a FAM field whose TYPE has the 
"counted_by" attribute with the new internal function ".ACCESS_WITH_SIZE";
* In every consumer of the size information, for example, BDOS or array bound 
sanitizer, query the size information or ACCESS_MODE information from the new 
internal function;
* When expansing to RTL, replace the internal function with the actual 
reference to the FAM field;
* Some adjustment to ipa alias analysis, and other SSA passes to mitigate the 
impact to the optimizer and code generation.


**The new internal function

  .ACCESS_WITH_SIZE (REF_TO_OBJ, REF_TO_SIZE, CLASS_OF_SIZE, TYPE_OF_SIZE, 
ACCESS_MODE, TYPE_OF_REF)

INTERNAL_FN (ACCESS_WITH_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)

which returns the "REF_TO_OBJ" same as the 1st argument;

Both the return type and the type of the first argument of this function have 
been converted from the incomplete array type to the corresponding pointer type.

The call to .ACCESS_WITH_SIZE is wrapped with an INDIRECT_REF, whose type is 
the original imcomplete array type.

Please see the following link for why:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638793.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639605.html

1st argument "REF_TO_OBJ": The reference to the object;
2nd argument "REF_TO_SIZE": The reference to the size of the object,
3rd argument "CLASS_OF_SIZE": The size referenced by the REF_TO_SIZE represents
   0: the number of bytes;
   1: the number of the elements of the object type;
4th argument "TYPE_OF_SIZE": A constant 0 with the TYPE of the object
  refed by REF_TO_SIZE
5th argument "ACCESS_MODE":
  -1: Unknown access semantics
   0: none
   1: read_only
   2: write_only
   3: read_write
6th argument "TYPE_OF_REF": A constant 0 with the pointer TYPE to
  to the original flexible array type.

** The Patch sets included:

1. Provide counted_by attribute to flexible array member field;
  which includes:
  * "counted_by" attribute documentation;
  * C FE handling of the new attribute;
syntax checking, error reporting;
  * testing cases;

2. Convert "counted_by" attribute to/from .ACCESS_WITH_SIZE.
  which includes:
  * The definition of the new internal function .ACCESS_WITH_SIZE in 
internal-fn.def.
  * C FE converts every reference to a FAM with "counted_by" attribute to a 
call to the internal function .ACCESS_WITH_SIZE.
(build_component_ref in c_typeck.cc)
This includes the case when the object is statically allocated and 
initialized.
In order to make this working, we should update 
initializer_constant_valid_p_1 and output_constant in varasm.cc to include 
calls to .ACCESS_WITH_SIZE.

However, for the reference inside "offsetof", ignore the "counted_by" 
attribute since it's not useful at all. (c_parser_postfix_expression in 
c/c-parser.cc)
In addtion to "offsetof", for the reference inside operator "typeof" and
  "alignof", we ignore counted_by attribute too.
When building ADDR_EXPR for the .ACCESS_WITH_SIZE in C FE,
  replace the call with its first argument.

  * Convert every call to .ACCESS_WITH_SIZE to its first argument.
(expand_ACCESS_WITH_SIZE in internal-fn.cc)
  * adjust alias analysis to exclude the new internal from clobbering 
anything.
(ref_maybe_used_by_call_p_1 and call_may_clobber_ref_p_1 in 
tree-ssa-alias.cc)
  * adjust dead code elimination to eliminate the call to .ACCESS_WITH_SIZE 
when
it's LHS is eliminated as dead code.
(el