linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Michal Hocko <mhocko@suse.com>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Lee Duncan <lduncan@suse.com>, Chris Leech <cleech@redhat.com>,
	Adam Nichols <adam@grimm-co.com>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2] seq_file: Unconditionally use vmalloc for buffer
Date: Tue, 16 Mar 2021 12:18:33 -0700	[thread overview]
Message-ID: <202103161208.22FC78C8C@keescook> (raw)
In-Reply-To: <YFCn4ERBMGoqxvUU@zeniv-ca.linux.org.uk>

On Tue, Mar 16, 2021 at 12:43:12PM +0000, Al Viro wrote:
> On Tue, Mar 16, 2021 at 08:24:50AM +0100, Greg Kroah-Hartman wrote:
> 
> > > Completely agreed. seq_get_buf() should be totally ripped out.
> > > Unfortunately, this is going to be a long road because of sysfs's ATTR
> > > stuff, there are something like 5000 callers, and the entire API was
> > > designed to avoid refactoring all those callers from
> > > sysfs_kf_seq_show().
> > 
> > What is wrong with the sysfs ATTR stuff?  That should make it so that we
> > do not have to change any caller for any specific change like this, why
> > can't sysfs or kernfs handle it automatically?
> 
> Hard to tell, since that would require _finding_ the sodding ->show()
> instances first.  Good luck with that, seeing that most of those appear
> to come from templates-done-with-cpp...

I *think* I can get coccinelle to find them all, but my brute-force
approach was to just do a debug build changing the ATTR macro to be
typed, and changing the name of "show" and "store" in kobj_attribute
(to make the compiler find them all).

> AFAICS, Kees wants to protect against ->show() instances stomping beyond
> the page size.  What I don't get is what do you get from using seq_file
> if you insist on doing raw access to the buffer rather than using
> seq_printf() and friends.  What's the point?

To me, it looks like the kernfs/sysfs API happened around the time
"container_of" was gaining ground. It's trying to do the same thing
the "modern" callbacks do with finding a pointer from another, but it
did so by making sure everything had a 0 offset and an identical
beginning structure layout _but changed prototypes_.

It's the changed prototypes that freaks out CFI.

My current plan consists of these steps:

- add two new callbacks to the kobj_attribute struct (and its clones):
  "seq_show" and "seq_store", which will pass in the seq_file.
- convert all callbacks to kobject/kboj_attribute and use container_of()
  to find their respective pointers.
- remove "show" and "store"
- remove external use of seq_get_buf().

The first two steps require thousands of lines of code changed, so
I'm going to try to minimize it by trying to do as many conversions as
possible to the appropriate helpers first. e.g. DEVICE_ATTR_INT exists,
but there are only 2 users, yet there appears to be something like 500
DEVICE_ATTR callers that have an open-coded '%d':

$ git grep -B10 '\bDEVICE_ATTR' | grep '%d' | wc -l
530

-- 
Kees Cook

  parent reply	other threads:[~2021-03-16 19:19 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-15 17:48 Kees Cook
2021-03-15 18:33 ` Al Viro
2021-03-15 20:43   ` Kees Cook
2021-03-16  7:24     ` Greg Kroah-Hartman
2021-03-16 12:43       ` Al Viro
2021-03-16 12:55         ` Greg Kroah-Hartman
2021-03-16 13:01         ` Michal Hocko
2021-03-16 19:18         ` Kees Cook [this message]
2021-03-17 10:44           ` Greg Kroah-Hartman
2021-03-16  8:31 ` Michal Hocko
2021-03-16 19:08   ` Kees Cook
2021-03-17 12:08     ` Michal Hocko
2021-03-17 13:34       ` Greg Kroah-Hartman
2021-03-17 14:44         ` Michal Hocko
2021-03-17 14:56           ` Greg Kroah-Hartman
2021-03-17 15:20             ` Michal Hocko
2021-03-17 15:38               ` Greg Kroah-Hartman
2021-03-17 15:48                 ` Michal Hocko
2021-03-17 21:30                 ` Kees Cook
2021-03-18  8:07                   ` Greg Kroah-Hartman
2021-03-18 15:51                     ` Kees Cook
2021-03-18 17:56                       ` Greg Kroah-Hartman
2021-03-19 14:07 ` [seq_file] 5fd6060e50: stress-ng.eventfd.ops_per_sec -49.1% regression kernel test robot
2021-03-19 19:31   ` Kees Cook

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=202103161208.22FC78C8C@keescook \
    --to=keescook@chromium.org \
    --cc=adam@grimm-co.com \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=cleech@redhat.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=lduncan@suse.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@suse.com \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v2] seq_file: Unconditionally use vmalloc for buffer' \
    /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

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