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
Date: Wed, 5 May 2021 14:29:16 -0300 [thread overview]
Message-ID: <20210505172916.GC2047089@ziepe.ca> (raw)
In-Reply-To: <YJLHHpatWOgJo0Zk@kroah.com>
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
next prev parent reply other threads:[~2021-05-05 17:57 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-02 19:52 CFI violation in drivers/infiniband/core/sysfs.c Nathan Chancellor
2021-04-02 23:03 ` Kees Cook
2021-04-02 23:30 ` Jason Gunthorpe
2021-04-03 1:29 ` Kees Cook
2021-04-04 13:57 ` Jason Gunthorpe
2021-05-05 16:26 ` Greg KH
2021-05-05 17:29 ` Jason Gunthorpe [this message]
2021-05-05 17:39 ` Greg KH
2021-04-03 6:55 ` Nathan Chancellor
2021-05-04 20:22 ` Jason Gunthorpe
2021-05-05 16:26 ` Greg KH
2021-05-05 20:08 ` Nathan Chancellor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210505172916.GC2047089@ziepe.ca \
--to=jgg@ziepe.ca \
--cc=clang-built-linux@googlegroups.com \
--cc=dledford@redhat.com \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=leon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=nathan@kernel.org \
--cc=parav@nvidia.com \
--cc=samitolvanen@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).