casaroli commented on code in PR #20764:
URL: https://github.com/apache/nuttx/pull/20764#discussion_r4307019709
##########
binfmt/builtin.c:
##########
@@ -115,11 +115,13 @@ static int builtin_loadbinary(FAR struct binary_s *binp,
binp->gid = builtin->gid;
binp->mode = builtin->mode;
- chk = binfmt_checkexecperm(binp);
+# ifdef CONFIG_BINFMT_CHECKEXEC
+ chk = binfmt_checkexecperm(binp->mode, binp->uid, binp->gid);
Review Comment:
The common check needs a file: it uses nx_stat() on the path. A builtin
application can be started by name without binfs, and then there is no file to
stat; its owner, group and mode come from the builtin table. When the builtin
is started through binfs, the common check covers it.
##########
binfmt/binfmt_loadmodule.c:
##########
@@ -103,10 +105,37 @@ static int load_absmodule(FAR struct binary_s *bin, FAR
const char *filename,
FAR const struct symtab_s *exports, int nexports)
{
FAR struct binfmt_s *binfmt;
+#ifdef CONFIG_BINFMT_CHECKEXEC
+ struct stat st;
+#endif
int ret = -ENOENT;
binfo("Loading %s\n", filename);
+#ifdef CONFIG_BINFMT_CHECKEXEC
+ /* Check the execute permission once, for every binary format. A name
+ * that is not a file (a builtin application without binfs) is checked
+ * by its loader.
+ */
+
+ if (nx_stat(filename, &st, 1) >= 0)
Review Comment:
Yes, it costs one more path lookup per exec(). The old checks used the mode
of the file that the ELF and NXFLAT loaders open anyway, but the script loader
had no check, and each new loader needs its own. I can move the check back into
the loaders (ELF, NXFLAT, builtin and script), each with the mode of its own
open file, if you prefer to avoid the extra lookup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
