Messages in this thread | | | From | (Eric W. Biederman) | Date | Fri, 25 Apr 2014 14:27:14 -0700 | Subject | Re: Kernel panic at Ubuntu: IMA + Apparmor |
| |
Dmitry Kasatkin <dmitry.kasatkin@gmail.com> writes:
> On 25 April 2014 23:45, Eric W. Biederman <ebiederm@xmission.com> wrote: >> Dmitry Kasatkin <dmitry.kasatkin@gmail.com> writes: >> >>> On 25 April 2014 23:01, Oleg Nesterov <oleg@redhat.com> wrote: >>>> On 04/25, Eric W. Biederman wrote: >>>>> >>>>> Oleg Nesterov <oleg@redhat.com> 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.
Am I wrong. Is it ok for IMA-appraisal to get permission denied when it wants to appraise a file?
Eric
| |