mbox series

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

Message ID 20240216194723.391359-1-qing.zhao@oracle.com
Headers show
Series New attribute "counted_by" to annotate bounds for C99 FAM(PR108896) | expand

Message

Qing Zhao Feb. 16, 2024, 7:47 p.m. UTC 
Hi,

This is the 6th version of the patch.

compare with the 5th version, the only difference is:

1. Add the 6th argument to .ACCESS_WITH_SIZE
   to carry the TYPE of the flexible array.
   Such information is needed during tree-object-size.cc.
    
   previously, we use the result type of the routine
   .ACCESS_WITH_SIZE to decide the element type of the
   original array, however, the result type of the routine
   might be changed during tree optimizations due to 
   possible type casting in the source code.


compare with the 4th version, the major difference are:

1. Change the return type of the routine .ACCESS_WITH_SIZE 
   FROM:
     Pointer to the type of the element of the flexible array;
   TO:
     Pointer to the type of the flexible array;
    And then wrap the call with an indirection reference. 

2. Adjust all other parts with this change, (this will simplify the bound sanitizer instrument);

3. Add the fixes to the kernel building failures, which include:
    A. The operator “typeof” cannot return correct type for a->array; 
    B. The operator “&” cannot return correct address for a->array;

4. Correctly handle the case when the value of “counted-by” is zero or negative as following
   4.1. Update the counted-by doc with the following:
    When the counted-by field is assigned a negative integer value, the compiler will treat the value as zero. 
   4.2. Adjust __bdos and array bound sanitizer to handle correctly when “counted-by” is zero. 


It based on the following 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: unknown;
   1: the number of the elements of the object type;
   2: the number of bytes;
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
  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.
        (eliminate_unnecessary_stmts in tree-ssa-dce.cc)
      * Provide the utility routines to check the call is .ACCESS_WITH_SIZE and
        get the reference from the call to .ACCESS_WITH_SIZE.
        (is_access_with_size_p and get_ref_from_access_with_size in tree.cc)
      * testing cases. (for offsetof, static initialization, generation of calls to
        .ACCESS_WITH_SIZE, code runs correctly after calls to .ACCESS_WITH_SIZE are
        converted back)

3. Use the .ACCESS_WITH_SIZE in builtin object size (sub-object only)
      which includes:
      * use the size info of the .ACCESS_WITH_SIZE for sub-object.
      * when the size is a negative integer, treat it as zero.
      * testing cases. 

4 Use the .ACCESS_WITH_SIZE in bound sanitizer
      which includes:
      * Instrument array_ref with a call to .ACCESS_WITH_SIZE for bound sanitizer.
      * when the size is a negative integer, treat it as zero.
      * testing cases. 

5. Add the 6th argument to .ACCESS_WITH_SIZE to carry the TYPE of the flexible array.
      which includes:
      * Add the 6th argument to .ACCESS_WITH_SIZE.
      * use the type of the 6th argument of the routine in tree-object-size.cc
      * testing case.

******Remaining works: 

6  Improve __bdos to use the counted_by info in whole-object size for the structure with FAM.
7  Emit warnings when the user breaks the requirments for the new counted_by attribute
   compilation time: -Wcounted-by
   run time: -fsanitizer=counted-by
      * The initialization to the size field should be done before the first reference to the FAM field.
      * the array has at least # of elements specified by the size field all the time during the program.

I have bootstrapped and regression tested on both x86 and aarch64, no issue.
Linux kernel linux-6.8-rc4 has been built and exposed one bug with the new counted-by, fixed.

Let me know your comments.

thanks.

Qing

Qing Zhao (5):
  Provide counted_by attribute to flexible array member field (PR108896)
  Convert references with "counted_by" attributes to/from
    .ACCESS_WITH_SIZE.
  Use the .ACCESS_WITH_SIZE in builtin object size.
  Use the .ACCESS_WITH_SIZE in bound sanitizer.
  Add the 6th argument to .ACCESS_WITH_SIZE

Comments

Kees Cook Feb. 16, 2024, 9:39 p.m. UTC | #1 
On Fri, Feb 16, 2024 at 07:47:18PM +0000, Qing Zhao wrote:
> This is the 6th version of the patch.

Thanks! I've tested this and it meets all the current behavioral
expectations I've got:
https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c

Additionally, this builds the Linux kernel where we have almost 300
instances of "counted_by" attributes.

Yay!

-Kees
Qing Zhao March 1, 2024, 2:38 p.m. UTC | #2 
Ping on this patch set.

Thanks a lot!

Qing

> On Feb 16, 2024, at 14:47, Qing Zhao <qing.zhao@oracle.com> wrote:
> 
> Hi,
> 
> This is the 6th version of the patch.
> 
> compare with the 5th version, the only difference is:
> 
> 1. Add the 6th argument to .ACCESS_WITH_SIZE
>   to carry the TYPE of the flexible array.
>   Such information is needed during tree-object-size.cc.
> 
>   previously, we use the result type of the routine
>   .ACCESS_WITH_SIZE to decide the element type of the
>   original array, however, the result type of the routine
>   might be changed during tree optimizations due to 
>   possible type casting in the source code.
> 
> 
> compare with the 4th version, the major difference are:
> 
> 1. Change the return type of the routine .ACCESS_WITH_SIZE 
>   FROM:
>     Pointer to the type of the element of the flexible array;
>   TO:
>     Pointer to the type of the flexible array;
>    And then wrap the call with an indirection reference. 
> 
> 2. Adjust all other parts with this change, (this will simplify the bound sanitizer instrument);
> 
> 3. Add the fixes to the kernel building failures, which include:
>    A. The operator “typeof” cannot return correct type for a->array; 
>    B. The operator “&” cannot return correct address for a->array;
> 
> 4. Correctly handle the case when the value of “counted-by” is zero or negative as following
>   4.1. Update the counted-by doc with the following:
>    When the counted-by field is assigned a negative integer value, the compiler will treat the value as zero. 
>   4.2. Adjust __bdos and array bound sanitizer to handle correctly when “counted-by” is zero. 
> 
> 
> It based on the following 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: unknown;
>   1: the number of the elements of the object type;
>   2: the number of bytes;
> 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
>  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.
>        (eliminate_unnecessary_stmts in tree-ssa-dce.cc)
>      * Provide the utility routines to check the call is .ACCESS_WITH_SIZE and
>        get the reference from the call to .ACCESS_WITH_SIZE.
>        (is_access_with_size_p and get_ref_from_access_with_size in tree.cc)
>      * testing cases. (for offsetof, static initialization, generation of calls to
>        .ACCESS_WITH_SIZE, code runs correctly after calls to .ACCESS_WITH_SIZE are
>        converted back)
> 
> 3. Use the .ACCESS_WITH_SIZE in builtin object size (sub-object only)
>      which includes:
>      * use the size info of the .ACCESS_WITH_SIZE for sub-object.
>      * when the size is a negative integer, treat it as zero.
>      * testing cases. 
> 
> 4 Use the .ACCESS_WITH_SIZE in bound sanitizer
>      which includes:
>      * Instrument array_ref with a call to .ACCESS_WITH_SIZE for bound sanitizer.
>      * when the size is a negative integer, treat it as zero.
>      * testing cases. 
> 
> 5. Add the 6th argument to .ACCESS_WITH_SIZE to carry the TYPE of the flexible array.
>      which includes:
>      * Add the 6th argument to .ACCESS_WITH_SIZE.
>      * use the type of the 6th argument of the routine in tree-object-size.cc
>      * testing case.
> 
> ******Remaining works: 
> 
> 6  Improve __bdos to use the counted_by info in whole-object size for the structure with FAM.
> 7  Emit warnings when the user breaks the requirments for the new counted_by attribute
>   compilation time: -Wcounted-by
>   run time: -fsanitizer=counted-by
>      * The initialization to the size field should be done before the first reference to the FAM field.
>      * the array has at least # of elements specified by the size field all the time during the program.
> 
> I have bootstrapped and regression tested on both x86 and aarch64, no issue.
> Linux kernel linux-6.8-rc4 has been built and exposed one bug with the new counted-by, fixed.
> 
> Let me know your comments.
> 
> thanks.
> 
> Qing
> 
> Qing Zhao (5):
>  Provide counted_by attribute to flexible array member field (PR108896)
>  Convert references with "counted_by" attributes to/from
>    .ACCESS_WITH_SIZE.
>  Use the .ACCESS_WITH_SIZE in builtin object size.
>  Use the .ACCESS_WITH_SIZE in bound sanitizer.
>  Add the 6th argument to .ACCESS_WITH_SIZE