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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210402233018.GA7721@ziepe.ca>
Date:   Fri, 2 Apr 2021 20:30:18 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Kees Cook <keescook@...omium.org>
Cc:     Nathan Chancellor <nathan@...nel.org>,
        Doug Ledford <dledford@...hat.com>,
        Leon Romanovsky <leon@...nel.org>,
        Parav Pandit <parav@...dia.com>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
        clang-built-linux@...glegroups.com
Subject: Re: CFI violation in drivers/infiniband/core/sysfs.c

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?

> > I would appreciate someone else taking a look and seeing if I am off
> > base or if there is an easier way to solve this.
> 
> So, it seems that the reason for a custom kobj_type here is to use the
> .release callback. 

Every kobject should be associated with a specific, fixed, attribute
type. The purpose of the wrappers is to inject type safety so the
attribute implementations know they are working on the right stuff.

In turn, an attribute of a specific attribute type can only be
injected into a kobject that has the matching type.

This code is insane because it does this:

		hsag->attrs[i] = alloc_hsa(i, port_num, stats->names[i]);

		// This is port kobject
		struct kobject *kobj = &port->kobj;
		ret = sysfs_create_group(kobj, hsag); 
[..]
                // This is a struct device kobject
		struct kobject *kobj = &device->dev.kobj;
		ret = sysfs_create_group(kobj, hsag); 

Which is absolutely not allowed, you can't have a generic attribute
handler and stuff it into two different types! Things MUST use the
proper attribute subclass for what they are being attached to.

The answer is that the setup_hw_stats_() for port and device must
be split up and the attribute implementations for each of them have to
subclass starting from the correct type.

So we'd end up with two attributes for hw_stats

struct port_hw_stats {
    struct port_attr attr;
    struct hw_stats_data data;
};

struct device_hw_stats {
    struct device_attr attr;
    struct hw_stats_data data;
};

And then two show/set functions that bounce through the correct types
to the data.

And so on.

Jason