Re: CFI violation in drivers/infiniband/core/sysfs.c

[Greg KH]

On Sun, Apr 04, 2021 at 10:57:13AM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 02, 2021 at 06:29:55PM -0700, Kees Cook wrote:
> > On Fri, Apr 02, 2021 at 08:30:18PM -0300, Jason Gunthorpe wrote:
> > > On Fri, Apr 02, 2021 at 04:03:30PM -0700, Kees Cook wrote:
> > > 
> > > > > relevant. It seems to me that the hw_counters 'struct attribute_group'
> > > > > should probably be its own kobj within both of these structures so they
> > > > > can have their own sysfs ops (unless there is some other way to do this
> > > > > that I am missing).
> > > 
> > > Err, yes, every subclass of the attribute should be accompanied by a
> > > distinct kobject type to relay the show methods with typesafety, this
> > > is how this design pattern is intended to be used.
> > > 
> > > If I understand your report properly the hw_stats_attribute is being
> > > assigned to a 'port_type' kobject and it only works by pure luck because
> > > the show/store happens to overlap between port and hsa attributes?
> > 
> > "happens to" :) Yeah, they're all like this, unfortunately:
> > https://lore.kernel.org/lkml/202006112217.2E6CE093@keescook/
> 
> All? I think these are all bugs, no?
> 
> struct kobj_attribute is only to be used with a kobj_sysfs_ops type
> kobject
> 
> To cross it over to a 'struct device' kobj is completely wrong, the
> same basic wrongness being done here.
>  
> > I'm not convinced that just backing everything off to kobject isn't
> > simpler?
> 
> It might be simpler, but isn't right - everything should continue to
> work after a patch like this:
> 
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -67,6 +67,7 @@ struct ib_port {
>  
>  struct port_attribute {
>  	struct attribute attr;
> +	uu64 pad[2];

Ick, don't do that :(

>  	ssize_t (*show)(struct ib_port *, struct port_attribute *, char *buf);
>  	ssize_t (*store)(struct ib_port *, struct port_attribute *,
>  			 const char *buf, size_t count);
> 
> If it doesn't it is still broken.
> 
> Using container_of() with the wrong types is an unconditional
> error. A kasn test to catch this would be very cool (think like RTTI
> and dynamic_cast<>() in C++)
> 
> > > And then two show/set functions that bounce through the correct types
> > > to the data.
> > 
> > I'd like to make these things compile-time safe (there is not type
> > associated with use the __ATTR() macro, for example). That I haven't
> > really figured out how to do right.
> 
> They are in many places, for instance.
> 
> int device_create_file(struct device *dev,
>                        const struct device_attribute *attr)
> 
> We loose the type safety when working with attribute arrays, and
> people can just bypass the "proper" APIs to raw sysfs ones whenever
> they like.
> 
> It is fundamentally completely wrong to attach a 'struct
> kobject_attribute' to a 'struct device' kobject.

But it works because we are using C and we don't have RTTI :)

Yes, it's horrid, but we do it because we "know" the real type that is
being called here.  That was an explicit design decision at the time.

If that was a good decision or not, I don't know, but it's served us
well for the past 20 years or so...

thanks,

greg k-h