Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 5 May 2021 14:29:16 -0300
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Kees Cook <keescook@chromium.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Doug Ledford <dledford@redhat.com>,
        Leon Romanovsky <leon@kernel.org>,
        Parav Pandit <parav@nvidia.com>,
        Sami Tolvanen <samitolvanen@google.com>,
        linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org,
        clang-built-linux@googlegroups.com
Subject: Re: CFI violation in drivers/infiniband/core/sysfs.c
Message-ID: <20210505172916.GC2047089@ziepe.ca>
References: <20210402195241.gahc5w25gezluw7p@archlinux-ax161>
 <202104021555.08B883C7@keescook>
 <20210402233018.GA7721@ziepe.ca>
 <202104021823.64FA6119@keescook>
 <20210404135713.GB7721@ziepe.ca>
 <YJLHHpatWOgJo0Zk@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YJLHHpatWOgJo0Zk@kroah.com>
Precedence: bulk

On Wed, May 05, 2021 at 06:26:06PM +0200, Greg KH wrote:
> > 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.

I think it is beyond horrid. Just so everyone is clear on what is
happening here..

RDMA has this:

struct hw_stats_attribute {
	struct attribute	attr;
	ssize_t	(*show)(struct kobject *kobj,
			struct attribute *attr, char *buf);

And it has two kobject types, a struct device kobject and a ib_port
kobject.

When the user invokes show on the struct device sysfs we have this
call path:

dev_sysfs_ops
  dev_attr_show()
    struct device_attribute *dev_attr = to_dev_attr(attr);
      ret = dev_attr->show(dev, dev_attr, buf); 
        show_hw_stats()
          struct hw_stats_attribute *hsa = container_of(attr, struct hw_stats_attribute, attr)

And from the ib_port kobject we have this one:

port_sysfs_ops
  port_attr_show()
    struct port_attribute *port_attr =
      container_of(attr, struct port_attribute, attr);
       	return port_attr->show(p, port_attr, buf);
          show_hw_stats()
           struct hw_stats_attribute *hsa = container_of(attr, struct hw_stats_attribute, attr)

Then show_hw_stats() goes on to detect which call chain it uses so it
can apply the proper container of to the kobj:

	if (!hsa->port_num)
		dev = container_of((struct device *)kobj,
				   struct ib_device, dev);
	else
		port = container_of(kobj, struct ib_port, kobj);

There are several nasty defeats of the C typing system here:

 - A hw_stats_attribute is casted to device_attribute hidden inside
   container_of()

 - The 'show' function pointer is being casted from from a
     (*show)(kobject,attr,buf) to (*show)(device,device_attr,buf)
   This cast is hidden by the above wrong use of container_of()

 - The dev_attr 'struct device_attribute *' is casted directly to a
   'struct attribute *' and this cast is hidden because of the wrongly type
   function pointer

 - The dev 'struct device *' is casted directly to a 'struct kobject *'
   and like above this is hidden inside the wrongly typed function
   pointer.

 - All of the above is true again when talking about port_attribute
   and struct ib_port.

This all implicitly relies on the following unchecked and undocumated
relationships:
 - struct device's kobject is the first member in the struct
 - struct ib_port's kobject is the first member in the struct
 - The attr, show and store members are at the same offset
   in struct device_attribute and struct hw_stats_attribute

None of this is even slightly clear from the code. If Nathan hadn't
pointed it out I don't think anyone would have known..

> 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...

I agree with Kees, "my mind rebelled". I don't think it aligned with
the modern kernel style. If tooling starts to shine light on these
bast casts I feel it would only improve code quality.

For instance the patch Kees pointed at e6d701dca989 ("ACPI: sysfs: Fix
pm_profile_attr type")

This is a legitimate typing bug. ACPI should not have been using
struct device_attribute with a kobject creted by

  acpi_kobj = kobject_create_and_add("acpi", firmware_kobj);

Certainly this RDMA code has no buisness being written like this
either, it nets out to saving about 50 lines of straightforward
duplicated code for a lot of worse junk.

Regards,
Jason