On Mon, May 26, 2014 at 07:21:54PM +0100, Djalal Harouni wrote:
> On Mon, May 26, 2014 at 10:59:10AM -0700, Andy Lutomirski wrote:
> > On Mon, May 26, 2014 at 10:41 AM, Djalal Harouni <tixxdz@xxxxxxxxxx> wrote:
> > > On Mon, May 26, 2014 at 10:01:20AM -0700, Andy Lutomirski wrote:
> > >> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@xxxxxxxxxx> wrote:
> > >> > This patch is preparation, it adds a couple of helpers to read data and
> > >> > to get the cached permission checks during that ->read().
> > >> >
> > >> > Currently INF entries share the same code, they do not implement
> > >> > specific ->open(), only ->read() coupled with callback calls. Doing
> > >> > permission checks during ->open() will not work and will only disturb
> > >> > the INF entries that do not need permission checks. Yes not all the INF
> > >> > entries need checks, the ones that need protections are listed below:
> > >> > /proc/<pid>/wchan
> > >> > /proc/<pid>/syscall
> > >> > /proc/<pid>/{auxv,io} (will be handled in next patches).
> > >> >
> > >> > So to have proper permission checks convert this INF entries to REG
> > >> > entries and use their open() and read() handlers to implement these
> > >> > checks. To achieve this we add the following helpers:
> > >> >
> > >> > * proc_read_from_buffer() a wrapper around simple_read_from_buffer(), it
> > >> > makes sure that count <= PROC_BLOCK_SIZE (3*1024)
> > >> >
> > >> > * pid_entry_read(): it will get a free page and pass it to the specific
> > >> > /proc/<pid>/$entry handler (callback). The handler is of the same
> > >> > types of the previous INF handlers, it will only receive an extra
> > >> > "permitted" argument that contains the cached permission check that
> > >> > was performed during ->open().
> > >> >
> > >> > The handler is of type:
> > >> > typedef int (*proc_read_fn_t)(char *page,
> > >> > struct task_struct *task, int permitted);
> > >>
> > >> [...]
> > >>
> > >> This strikes me as *way* too complicated. Why not just change
> > >> proc_info_file_operations to *always* cache may_ptrace on open?
> > > Not all the INF entries need permission checks during open:
> > > The one that need it:
> > > /proc/<pid>/{wchan|syscall} converted in this series.
> > > /proc/<pid>/{auxv|io} (will be converted in next series)
> > >
> > > The ones that do not need it:
> > > /proc/<pid>/{limite|cmdline|shedstat|oom_score|...}
> > >
> > > There is no reason to do it for these entries, and if you do it you will
> > > also have to passe the cached permission checks, so I don't think that
> > > you want to modify all the INF handlers especially the ones that do not
> > > need it to take an extra argument.
> >
> > ...but you don't have to pass the checks, because nothing will care
> > whether you passed them.
> No one will use them, but what you suggest is passing a dummy value that
> no one cares about!
>
> You said in the first place that this is *way* too complicated, no IHMO
> going and changing all the handlers to receive a dummy argument that no one
> will use is more complicated. In this case you are just suggesting to
> convert all the INF entries into REG ones. Andi take a look please!
>
> The definition of INF can't get a 'struct file_operations', only the
> REG, at the end what you suggest is change all the INF entries to REG
> and do a ptrace check at open() that only a small subset of these
> entries will use...
>
> Try it and you will see how much code you will change.
>
> While the current proposed change just introduce a new helper (new code)
> and adds the cached permission argument!
>
> > > Then you will also modify this:
> > > in file fs/proc/internal.h the:
> > > union proc_op {
> > > ...
> > > int (*proc_read)(struct task_struct *task, char *page);
> > > ...
> > > }
> > >
> > > And then you will follow in all other places and handlers... and modify
> > > the definition of INF entries... It's much more complicated...
> > >
> >
> > But the final result will be much less messy and have less duplicated
> > code. Your patch seems like it duplicates a bunch of code and I
> > suspect that Al Viro will dislike it if he reads it.
> Hmm, as I've said if we do that we need to convert all the INF entries
> to REG ones, and remove the INF definition since it will be useless, then
> convert all the handlers... And I can't consider it duplicate code,
> since one is to handle non-sensitive entries and the other to handle
> sensitive ones, perhaps write a new function that will get the small
> duplicate instructions.
>
>
> For Al Viro oh thank you :-), hope that he reads the patches first, then
> if he thinks that converting all the INF entries into REG ones, and
> doing ptrace check during ->open for files that do not need it is the
> best solution, well I'll have just to follow and update the patches :-D
If I did convert all the INF to REG, well I'm sure that no one will like
the git diff stat in the first place, at least this way they will first
look at the patches and judge :-)
IIRC, Al Viro didn't like the f_cred in the first place since it touches
the seq struct, he said that it does not belong there!
So the current code is much contained, it just touches
/proc/<pid>/$entries, and I'll try to see if we can share some duplicate
instructions into a single new function!
Note that the duplication you point, is in this case is used for
sensitive files and non-sensitive files:
the old proc_info_read() and the new pid_entry_read() patch 3/9
the old proc_single_show() and the new pid_entry_show() patch 7/9
I did this to avoid changing the handlers and what was explained in the
previous email, Ahh and to reduce the git diff stat!
I'm open to proposition!
Thanks!
> Thank you Andi!
>
>
> > --Andy
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Djalal Harouni
> http://opendz.org
--
Djalal Harouni
http://opendz.org
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
