Messages in this thread | | | From | David Hildenbrand <> | Subject | Re: [PATCH v1 3/7] kernel/fork: always deny write access to current MM exe_file | Date | Thu, 12 Aug 2021 21:38:26 +0200 |
| |
On 12.08.21 18:51, Linus Torvalds wrote: > On Wed, Aug 11, 2021 at 10:45 PM David Hildenbrand <david@redhat.com> wrote: >> >> /* No ordering required: file already has been exposed. */ >> - RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm)); >> + exe_file = get_mm_exe_file(oldmm); >> + RCU_INIT_POINTER(mm->exe_file, exe_file); >> + if (exe_file) >> + deny_write_access(exe_file); > > Can we make a helper function for this, since it's done in two different places?
Sure, no compelling reason not to (except finding a suitable name, but I'll think about that tomorrow).
> >> - if (new_exe_file) >> + if (new_exe_file) { >> get_file(new_exe_file); >> + /* >> + * exec code is required to deny_write_access() successfully, >> + * so this cannot fail >> + */ >> + deny_write_access(new_exe_file); >> + } >> rcu_assign_pointer(mm->exe_file, new_exe_file); > > And the above looks positively wrong. The comment is also nonsensical, > in that it basically says "we thought this cannot fail, so we'll just > rely on it".
Well, it documents the expectation towards the caller, but in a suboptimal way, I agree.
> > If it truly cannot fail, then the comment should give the reason, not > the "we depend on this not failing".
Right, "We depend on the caller already have done a deny_write_access() successfully first such that this call cannot fail." combined with
if (deny_write_access(new_exe_file)) pr_warn("Unexpected failure of deny_write_access() in %s", __func__);
suggestions welcome.
> > And honestly, I don't see why it couldn't fail. And if it *does* fail, > we cannot then RCU-assign the exe_file pointer with this, because > you'll get a counter imbalance when you do the allow_write_access() > later.
Anyone calling set_mm_exe_file() (-> begin_new_exec()) is expected to successfully triggered a deny_write_access() upfront such that we won't fail at that point.
Further, on the dup_mmap() path we are sure the previous oldmm exe_file properly saw a successful deny_write_access() already, because that's now guaranteed for any exe_file.
> > Anyway, do_open_execat() does do deny_write_access() with proper error > checking. I think that is the existing reference that you depend on - > so that it doesn't fail. So the comment could possibly say that the > only caller has done this, but can we not just use the reference > deny_write_access() directly, and not do a new one here?
I think that might over-complicate the exec code where we would see a allow_write_access() on error paths, but not on success paths. This here looks cleaner to me, agreeing that the comment and the error check has to be improved.
We handle all allow_write_access()/deny_write_access() regarding exe_file completely in kernel/fork.c, which is IMHO quite nice.
> > IOW, maybe there's an extraneous 'allow_write_access()' somewhere that > should be dropped when we do the whole binprm dance in execve()?
fs/exec.c: free_bprm() and exec_binprm() to be precise.
Thanks!
-- Thanks,
David / dhildenb
| |