Re: CFI support for type punning?

Kent Overstreet
2023-11-18T20:27:39Z

On Fri, Nov 17, 2023 at 09:07:06PM -0800, Kees Cook wrote:
> There is no type punning support, though a neighboring idea has been
> proposed to define separate groups of the same prototypes (e.g. so one
> work_struct user can't call another work_struct user's callbacks). But
> that's all future work.

There was a proposal for making container_of() typesafe awhile back (I
forget who came up with it - Matthew, maybe?). Related?

>
> > The alternatives I'm looking at aren't terribly palatable; we'd either
> > have to increase the size of struct closure with another function
> > pointer (and another indirect function call on every use); or we'd have
> > to patch the workqueue code and add a flag (Tejun won't like that), or
> > I'll have to convert every closure function to take a work_struct arg -
> > with a resulting decrease in clarity of types.
>
> I think the reason the existing compiler warnings we have in place for
> this didn't warn was because there was no explicit casting; it was all
> hiding in an intentionally overlapped union. :)

;)

> struct closure {
> union {
> struct {
> struct workqueue_struct *wq;
> struct closure_syncer *s;
> struct llist_node list;
> closure_fn *fn;
> };
> struct work_struct work;
> };
>
> ...
> BUILD_BUG_ON(offsetof(struct closure, fn)
> != offsetof(struct work_struct, func));
>
> It seems the assignment used "fn", and the WORK_INIT used its internal
> func for the call. No casting needed. :)
>
> The good news is that closure.h _is_ using the common code pattern for
> these kinds of things (container_of() to find the wrapping struct),
> but it's using "struct closure" instead of "struct work_struct", which
> is what you concluded as well.
>
> Part of the work we did over the last few years now was fixing cases
> like this (where the wrapping function was being passed instead of
> the work_struct), and it just needed a few localized refactorings
> that Coccinelle seemed to handle well. I don't think it should be too
> disruptive here either.
>
> static void journal_write_done(struct closure *cl)
> {
> struct journal *j = container_of(cl, struct journal, io);
>
> Would, I think, become:
>
> static void journal_write_done(struct work_struct *ws)
> {
> struct closure *cl = container_of(ws, struct work_struct, work);
> struct journal *j = container_of(cl, struct journal, io);

The thing I don't like about this change is that closure fns are
significantly semantically different from a normal workqueue function -
a closure fn runs with a refcount held on the closure that it's
responsible for releasing either via continue_at() or closure_return().

> Usually these kinds of changes resulted in no binary differences (as
> expected). I do agree, though, the ergonomics are a little weird to
> expose the internals of closure.h. I've seen other APIs paper over this
> in their own special ways. For example, maybe something like this:
>
> #define CLOSURE_CALLBACK(name) \
> static void name(struct work_struct *ws)
>
> #define closure_type(type, member, name) \
> type *name = container_of(ws, struct work_struct, member.work)
>
> CLOSURE_CALLBACK(journal_write_done)
> {
> closure_type(struct journal, io, j);

Yeah, I think this works.

> And another, which was quite painful, was the struct timer_list work in
> 2018, which touched something like 1000 files. :|
> https://outflux.net/blog/archives/2018/02/05/security-things-in-linux-v4-15/#v4.15-timer_list

Thanks for the links :)

Related to the timer list change - back when I was active in the block
layer I was contemplating getting rid of bi_private, in favour of
container_of(). I wonder if that's something we could start nudging
people to do, less pointer chasing is always good :)