From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Subject: Re: [PATCH 3/9] procfs: add proc_read_from_buffer() and pid_entry_read() helpers Date: Mon, 26 May 2014 10:59:10 -0700 Message-ID: References: <1401110850-3552-1-git-send-email-tixxdz@opendz.org> <1401110850-3552-4-git-send-email-tixxdz@opendz.org> <20140526174100.GB6380@dztty> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Kees Cook , Andrew Morton , Alexey Dobriyan , "Eric W. Biederman" , Al Viro , Linus Torvalds , Ingo Molnar , Oleg Nesterov , Peter Zijlstra , LKML , Linux FS Devel To: Djalal Harouni Return-path: In-Reply-To: <20140526174100.GB6380@dztty> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Mon, May 26, 2014 at 10:41 AM, Djalal Harouni 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 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//wchan >> > /proc//syscall >> > /proc//{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//$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//{wchan|syscall} converted in this series. > /proc//{auxv|io} (will be converted in next series) > > The ones that do not need it: > /proc//{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. > 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. --Andy