On 26 April 2014 00:46, Dmitry Kasatkin <[email protected]> wrote: > On 26 April 2014 00:27, Eric W. Biederman <[email protected]> wrote: >> Dmitry Kasatkin <[email protected]> writes: >> >>> On 25 April 2014 23:45, Eric W. Biederman <[email protected]> wrote: >>>> Dmitry Kasatkin <[email protected]> writes: >>>> >>>>> On 25 April 2014 23:01, Oleg Nesterov <[email protected]> wrote: >>>>>> On 04/25, Eric W. Biederman wrote: >>>>>>> >>>>>>> Oleg Nesterov <[email protected]> writes: >>>>>>> >>>>>>> > Well. I _think_ that __fput() and ima_file_free() in particular >>>>>>> > should not >>>>>>> > depend on current and/or current->nsproxy. If nothing else, fput() >>>>>>> > can be >>>>>>> > called by the unrelated task which looks into /proc/pid/. >>>>>>> > >>>>>>> > But again, task_work_add() has more and more users, and it seems that >>>>>>> > even >>>>>>> > __fput() paths can do "everything", so perhaps it would be safer to >>>>>>> > allow >>>>>>> > to use ->nsproxy in task_work_run. >>>>>>> >>>>>>> Like I said, give me a clear motivating case. >>>>>> >>>>>> I agree, we need a reason. Currently I do not see one. >>>>>> >>>>>>> Right now not allowing >>>>>>> nsproxy is turning up bugs in __fput. Which seems like a good thing. >>>>>> >>>>>> This is what I certainly agree with ;) >>>>>> >>>>> >>>>> Hi, >>>>> >>>>> IMA uses kernel_read API which does not know anything about caller. >>>>> And of course security frameworks are at guard as usual. >>>>> >>>>> Exactly after reading first Eric's respons, I thought why to scratch >>>>> the head when task work queues are indeed designed for tasks... >>>> >>>> __fput has no guarantee of running in the task that close the file >>>> descriptor. If your code depends on that your code is broken. >>>> >>>>> And if you to dig for the history, IMA-appraisal was stuck due to >>>>> lockdep reporting even though it was on non-everlaping cases. >>>>> IIRC files vs. directories... >>>>> >>>>> After that IIRC Al Viro discussed about delayed fput and IIRC Oleg >>>>> (sorry if I am wrong) introduced task work queues. >>>>> >>>>> So IMA-appraisal was able to be upstreamed... That was ~3.4 time frame, >>>>> IIRC >>>>> >>>>> Name space also dated around ~3.4?? >>>>> Apparmor namespace change was also around that time. >>>>> >>>>> 3.10 introduces this name space order change and broke IMA-appraisal. >>>> >>>> IMA-appraisal is fundamentally broken because I can take a mandatory >>>> file lock and prevent IMA-apprasial. >>>> >>> >>> What file lock are you talking about? >>> IMA-appraisal does not depends on file locks... >> >> It honors them. Look at rw_verify_area, in vfs_read, in kernel_read. >> >> It sure looks like locks_mandatory_area can cause your kernel_read to >> fail. >> >>>> Using kernel_read is what allows this. >>>> >>>>> Isn't it a clear motivating case??? >>>> >>>> kernel_read is not appropriate for IMA use. The rest of this is just >>>> the messenger. >>>> >>>> IMA needs to use a cousin of kernel_read that operates at a lower level >>>> than vfs_read. A function that all of the permission checks and the >>>> fsnotify work. >>>> >>>> I am sorry to be the bearer of bad news. But kernel_read is totally >>>> inappropriate for IMA. >>>> >>> >>> So you break IMA-appraisal and declare that it cannot be used now? >> >> I didn't break it. I read the code, and I read the back trace to see >> where the bug was. >> >> I see IMA-appraisal trying to read file data as if it were a user space >> application in such a way that it can get permission denied for a whole >> host of reasons. >> >> My understanding of IMA-appraisal is that using a code path that can >> give use permission denined when performing appraisal is a way for >> clever people to attack and avoid IMA-appraisal without violating any >> security policy. >> > > Interesting thing is that file was already open before and LSM gave OK for > this. > Then re-reading the file on close in fact does not need any LSM > permission checks. > But as kernel_read API is still the same, it goes via the same checks... > > But on close with delayed fput nsproxy is missing .... > >> Am I wrong. Is it ok for IMA-appraisal to get permission denied when it >> wants to appraise a file? >> > > IMA is called after may_open... > > >> Eric > >
Is it really a show stopper to switch order of 2 functions as quick fix? It was like that before 3.10 and seemed ok... > > -- > Thanks, > Dmitry -- Thanks, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
