Messages in this thread | | | From | Daniel Verkamp <> | Date | Fri, 2 Dec 2022 15:23:41 -0800 | Subject | Re: [PATCH v3] mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd |
| |
On Thu, Dec 1, 2022 at 5:36 PM <jeffxu@chromium.org> wrote: > > From: Jeff Xu <jeffxu@chromium.org> > > When apply F_SEAL_EXEC to an executable memfd, add write seals also to > prevent modification of memfd. > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > --- > mm/memfd.c | 3 +++ > tools/testing/selftests/memfd/memfd_test.c | 25 ++++++++++++++++++++++ > 2 files changed, 28 insertions(+) > > diff --git a/mm/memfd.c b/mm/memfd.c > index 96dcfbfed09e..3a04c0698957 100644 > --- a/mm/memfd.c > +++ b/mm/memfd.c > @@ -222,6 +222,9 @@ static int memfd_add_seals(struct file *file, unsigned int seals) > } > } > > + if (seals & F_SEAL_EXEC && inode->i_mode & 0111) > + seals |= F_ALL_SEALS; > + > *file_seals |= seals; > error = 0; >
Hi Jeff,
(Following up on some discussion on the original review, sorry for any duplicate comments.)
Making F_SEAL_EXEC imply all seals (including F_SEAL_SEAL) seems a bit confusing. This at least needs documentation to make it clear.
Rather than silently adding other seals, perhaps we could return an error if the caller requests F_SEAL_EXEC but not the write seals, so the other seals would have to be explicitly listed in the application code. This would have the same net effect without making the F_SEAL_EXEC operation too magical.
Additionally, if the goal is to enforce W^X, I don't think this completely closes the gap. There will always be a period where it is both writable and executable with this API:
1. memfd_create(MFD_EXEC). Can't use MFD_NOEXEC since that would seal chmod(+x), so the memfd is W + X here. 2. write() code to the memfd. 3. fcntl(F_ADD_SEALS, F_SEAL_EXEC) to convert the memfd to !W + X.
I think one of the attack vectors involved the attacker waiting for another process to create a memfd, pausing/delaying the victim process, overwriting the memfd with their own code, and calling exec() on it, which is still possible in the window between steps 1 and 3 with this design.
Thanks, -- Daniel
| |