On Tue, Aug 27, 2024 at 6:58 AM Qing Zhao <qing.z...@oracle.com> wrote:
> > On Aug 27, 2024, at 02:17, Martin Uecker <uec...@tugraz.at> wrote:
> > Am Montag, dem 26.08.2024 um 17:21 -0700 schrieb Kees Cook:
> >> On Mon, Aug 26, 2024 at 11:01:08PM +0200, Martin Uecker wrote:
> >>> Am Montag, dem 26.08.2024 um 13:30 -0700 schrieb Kees Cook:
> >>>> On Mon, Aug 26, 2024 at 07:30:15PM +0000, Qing Zhao wrote:
> >>>>> Hi, Martin,
> >>>>>
> >>>>> Looks like that there is some issue when I tried to use the _Generic
> >>>>> for the testing cases, and then I narrowed down to a
> >>>>> small testing case that shows the problem without any change to GCC.
> >>>>>
> >>>>> [opc@qinzhao-ol8u3-x86 gcc]$ cat t1.c
> >>>>> struct annotated {
> >>>>> char b;
> >>>>> int c[];
> >>>>> } *array_annotated;
> >>>>> extern void * counted_by_ref (int *);
> >>>>>
> >>>>> int main(int argc, char *argv[])
> >>>>> {
> >>>>> typeof(counted_by_ref (array_annotated->c)) ret
> >>>>> = counted_by_ref (array_annotated->c);
> >>>>> _Generic (ret, void* : (void)0, default: *ret = 10);
> >>>>>
> >>>>> return 0;
> >>>>> }
> >>>>> [opc@qinzhao-ol8u3-x86 gcc]$ /home/opc/Install/latest/bin/gcc t1.c
> >>>>> t1.c: In function ‘main’:
> >>>>> t1.c:12:44: warning: dereferencing ‘void *’ pointer
> >>>>> 12 | _Generic (ret, void* : (void)0, default: *ret = 10);
> >>>>> | ^~~~
> >>>>> t1.c:12:49: error: invalid use of void expression
> >>>>> 12 | _Generic (ret, void* : (void)0, default: *ret = 10);
> >>>>> | ^
> >>>>
> >>>> I implemented it like this[1] in the Linux kernel. So yours could be:
> >>>>
> >>>> struct annotated {
> >>>> char b;
> >>>> int c[] __attribute__((counted_by(b));
> >>>> };
> >>>> extern struct annotated *array_annotated;
> >>>>
> >>>> int main(int argc, char *argv[])
> >>>> {
> >>>> typeof(_Generic(__builtin_get_counted_by(array_annotated->c),
> >>>> void *: (size_t *)NULL,
> >>>> default: __builtin_get_counted_by(array_annotated->c)))
> >>>> ret = __builtin_get_counted_by(array_annotated->c);
> >>>> if (ret)
> >>>> *ret = 10;
> >>>>
> >>>> return 0;
> >>>> }
> >>>>
> >>>> It's a bit cumbersome, but it does what's needed.
> >>>>
> >>>> This is, however, just doing exactly what Bill has suggested: it is
> >>>> converting the (void *)NULL into (size_t *)NULL when there is no
> >>>> counted_by annotation...
> >>>>
> >>>> -Kees
> >>>>
> >>>> [1]
> >>>> https://lore.kernel.org/linux-hardening/20240822231324.make.666-k...@kernel.org/
> >>>
> >>> Interesting. Will __builtin_get_counted_by(array_annotated->c) give
> >>> a null pointer (or an invalid pointer) of the correct type if
> >>> array_annotated is a null pointer of an annotated struct type?
> >>
> >> If you mean this part:
> >>
> >> typeof(P) __obj_ptr = NULL; \
> >> /* Just query the counter type for type_max checking. */ \
> >> typeof(_Generic(__flex_counter(__obj_ptr->FAM), \
> >> void *: (size_t *)NULL, \
> >> default: __flex_counter(__obj_ptr->FAM))) \
> >> __counter_type_ptr = NULL; \
> >>
> >> Where __obj_ptr starts as NULL, then yes. (Or at least, yes it does
> >> currently with Qing's GCC patch and Bill's Clang patch.)
> >
> > Does __builtin_get_counted_by not evaluate its argument?
>
> __builtin_get_counted_by currently is implemented as a C reserved words, and
> purely implemented in C parser as an C operator.
>
> So, it doesn’t apply complicated evaluations on its argument.
> I think that this should provide enough and simple functionality as needed.
> If no, please let me know.
>
>
> > In any
> > case, I think this should be documented whether this is
> > supposed to work (or not).
> Okay.
> >
> >>
> >>> I also wonder a bit about the multiple macro evaluations of the arguments
> >>> for P and SIZE.
> >>
> >> I tried to design it so they aren't used with anything that should
> >> have side-effects.
> >
> > I was more concerned about the cost of macro expansions on
> > compile times. I would do:
> >
> > __auto_type __FOO = (FOO);
> >
> > for all macro parameters that are evaluated multiple times
> > and are expressions which might contain macros themselves.
> >
> > There is also the issue of evaluation of typeof for variably modified
> > types, which might not currently affect the kernel, but this would
> > also become safer for such types.
> >
> >
> >> Anyway, if __builtin_get_counted_by returns (size_t *)NULL then I think
> >> the _Generic wrapping isn't needed. That would make it easier to use?
> >
> > It would make it easier for your use case. I wonder though
> > whether other people might want to have the compile time error
> > when there is no attribute.
>
> Then this will go back to the previous discussion: whether we should go the
> approach for the unary operator __counted_by(PTR), then the other builtin
> __builtin_has_attribute(PTR, counted_by) should be provided to the user.
>
> For GCC, there is no issue, we already have the __builtin_has_attribute (PTR,
> counted_by) available.
> But CLANG doesn’t have this builtin currently, need to implement it before
> the unary operator __counted_by(PTR).
>
> Let me know your opinion.
We have a bit of an issue with the current design. The group at Apple,
who are implementing a lot of the bounds safety checks need to prevent
the address of the 'count' from being taken. See [1] for details, but
the upshot is that they try to ensure that the 'count' and flexible
array member aren't modified independently. The discussion is ongoing,
but they suggested an alternative that is similar to ones discussed in
this thread and the bugzilla.
A brief description of their proposal is adding the builtin
'__builtin_bounds_attr_arg'. It would take counted_by's argument and
return the counter directly:
__builtin_choose_expr(
__builtin_has_attribute(__p->FAM, "counted_by"),
__builtin_bounds_attr_arg(__p->FAM) = COUNT,
(void)
);
This is similar to one of the original proposals for
__builtin_get_counted_by, but we changed because Clang didn't have
__builtin_has_attribute. Clang should implement
__builtin_has_attribute regardless, and if this is a better way of
doing things, and doesn't interfere with Apple's work, then I can add
__builtin_has_attribute to Clang so that we "do the right thing."
As I said though, the discussion is ongoing at [1], but just wanted to
give everyone a warning, and feel free to jump in with comments /
suggestions.
[1]
https://discourse.llvm.org/t/rfc-introducing-new-clang-builtin-builtin-get-counted-by/80836
-bw
