dmz: cloned binary: set +x permissions when creating regular tmpfile #4444
Conversation
| if err := file.Chmod(0o511); err != nil { | ||
| return nil, nil, fmt.Errorf("chmod classic tmpfile: %w", err) | ||
| } |
There was a problem hiding this comment.
Isn't this already done in sealFile?
There was a problem hiding this comment.
Yes, but because of isExecutable check, we will get an error because we can't find any executable file after we traversed all the tmpDirs:
runc/libcontainer/dmz/cloned_binary_linux.go
Lines 165 to 169 in e2647bd
There was a problem hiding this comment.
Hmm, okay so we should move the code from sealFile then rather than duplicating it. Also we should do this after we do os.Remove() so if there is an error we don't leak the temporary file.
There was a problem hiding this comment.
It would be nice to explain the issue in the commit message btw 😸, something like:
dmz: cloned binary: set +x permissions when creating regular tmpfile
While we did set +x when "sealing" regular temporary files, the "is
executable" checks were done before then and would thus fail, causing
the fallback to not work properly.
So just set +x after we create the file. We already have a O_RDWR handle
open when we do the chmod so we won't get permission issues when writing
to the file.
Fixes: e089db3b4a31 ("dmz: add fallbacks to handle noexec for O_TMPFILE and mktemp()")
Signed-off-by: lifubang <lifubang@acmcoder.com>
While we did set +x when "sealing" regular temporary files, the "is executable" checks were done before then and would thus fail, causing the fallback to not work properly. So just set +x after we create the file. We already have a O_RDWR handle open when we do the chmod so we won't get permission issues when writing to the file. Fixes: e089db3 ("dmz: add fallbacks to handle noexec for O_TMPFILE and mktemp()") Signed-off-by: lifubang <lifubang@acmcoder.com>
lifubang
force-pushed
the
fix-mktemp-exec
branch
from
e2647bd to
9fa324c
Compare
lifubang
changed the title
There was a problem hiding this comment.
@lifubang how have you found this? Is this a very old kernel?
If yes, can you check what error otmpfile returns?
I'm thinking, if older kernels return EISDIR, we should not retry otmpfile with a different directory, but jump straight to mktemp fallback.
Of course, this is not really related to this PR and can be done separately.
No, it's from code review. I find there may be an error, so I delete the memfd and otmpfile code to test it. |
Though creates a classic temp file for runc binary is the last fallback method of
CloneBinary, we should also fix the error of it.