This is an automated email from the ASF dual-hosted git repository.
GUIDINGLI pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new 5b383ae5baa binfmt: copy argv and file actions once, bounded
5b383ae5baa is described below
commit 5b383ae5baa0a9e5ccdcd00f146e29d9f3ccc22c
Author: Royyan Zahir <[email protected]>
AuthorDate: Wed Sep 30 15:07:03 2026 +0400
binfmt: copy argv and file actions once, bounded
Both copies sized the buffer in one pass and filled it in another,
rereading user memory; a second thread could lengthen a string or the
list between them and overflow the kernel heap.
Signed-off-by: Royyan Zahir <[email protected]>
---
binfmt/binfmt_copyactions.c | 121 +++++++++++++++++++++++++-------------------
binfmt/binfmt_copyargv.c | 25 ++++++---
2 files changed, 87 insertions(+), 59 deletions(-)
diff --git a/binfmt/binfmt_copyactions.c b/binfmt/binfmt_copyactions.c
index 774d9bef597..99f0acbce31 100644
--- a/binfmt/binfmt_copyactions.c
+++ b/binfmt/binfmt_copyactions.c
@@ -38,6 +38,12 @@
#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL) &&
!defined(CONFIG_BINFMT_DISABLE)
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define MAX_FILE_ACTIONS 256
+
/****************************************************************************
* Public Functions
****************************************************************************/
@@ -64,17 +70,20 @@ int binfmt_copyactions(FAR const posix_spawn_file_actions_t
**copy,
FAR const posix_spawn_file_actions_t *actions)
{
FAR struct spawn_general_file_action_s *entry;
- FAR struct spawn_general_file_action_s *prev;
- FAR struct spawn_close_file_action_s *close;
+ FAR struct spawn_general_file_action_s *prev = NULL;
FAR struct spawn_open_file_action_s *open;
- FAR struct spawn_open_file_action_s *tmp;
- FAR struct spawn_dup2_file_action_s *dup2;
- FAR void *buffer;
- int size = 0;
-
+ FAR struct spawn_open_file_action_s *src;
+ enum spawn_file_actions_e action;
+ FAR char *buffer;
+ FAR char *end;
+ size_t size = 0;
+ size_t len;
+ int count = 0;
+ int i;
+
+ *copy = NULL;
if (actions == NULL)
{
- *copy = NULL;
return OK;
}
@@ -82,6 +91,11 @@ int binfmt_copyactions(FAR const posix_spawn_file_actions_t
**copy,
entry != NULL;
entry = entry->flink)
{
+ if (++count > MAX_FILE_ACTIONS)
+ {
+ return -EFAULT;
+ }
+
switch (entry->action)
{
case SPAWN_FILE_ACTION_CLOSE:
@@ -103,72 +117,75 @@ int binfmt_copyactions(FAR const
posix_spawn_file_actions_t **copy,
}
}
- *copy = buffer = kmm_malloc(size);
+ buffer = kmm_malloc(size);
if (buffer == NULL)
{
return -ENOMEM;
}
- /* We need to copy and re-organize the flink chain, be care not modify
- * the actions it self, the prev have to point to the last time foreach
- * item.
- */
+ *copy = (FAR const posix_spawn_file_actions_t *)buffer;
+ end = buffer + size;
+ entry = (FAR struct spawn_general_file_action_s *)actions;
- for (entry = (FAR struct spawn_general_file_action_s *)actions,
- prev = NULL; entry != NULL; entry = entry->flink)
+ for (i = 0; i < count; i++, entry = entry->flink)
{
- switch (entry->action)
+ if (entry == NULL)
+ {
+ goto errout;
+ }
+
+ action = entry->action;
+ switch (action)
{
case SPAWN_FILE_ACTION_CLOSE:
- close = buffer;
- memcpy(close, entry, sizeof(struct spawn_close_file_action_s));
- close->flink = NULL;
- if (prev)
- {
- prev->flink = (FAR void *)close;
- }
-
- prev = (FAR void *)close;
- buffer = close + 1;
+ len = sizeof(struct spawn_close_file_action_s);
break;
case SPAWN_FILE_ACTION_DUP2:
- dup2 = buffer;
- memcpy(dup2, entry, sizeof(struct spawn_dup2_file_action_s));
- dup2->flink = NULL;
- if (prev)
- {
- prev->flink = (FAR void *)dup2;
- }
-
- prev = (FAR void *)dup2;
- buffer = dup2 + 1;
+ len = sizeof(struct spawn_dup2_file_action_s);
break;
case SPAWN_FILE_ACTION_OPEN:
- tmp = (FAR struct spawn_open_file_action_s *)entry;
- open = buffer;
- memcpy(open, entry, sizeof(struct spawn_open_file_action_s));
- open->flink = NULL;
- if (prev)
- {
- prev->flink = (FAR void *)open;
- }
-
- strcpy(open->path, tmp->path);
-
- prev = (FAR void *)open;
- buffer = (FAR char *)buffer +
- ALIGN_UP(SIZEOF_OPEN_FILE_ACTION_S(strlen(tmp->path)),
- sizeof(FAR void *));
+ len = sizeof(struct spawn_open_file_action_s);
break;
default:
- break;
+ goto errout;
+ }
+
+ if (len > end - buffer)
+ {
+ goto errout;
+ }
+
+ memcpy(buffer, entry, len);
+ if (action == SPAWN_FILE_ACTION_OPEN)
+ {
+ open = (FAR struct spawn_open_file_action_s *)buffer;
+ src = (FAR struct spawn_open_file_action_s *)entry;
+ len = strnlen(src->path, end - buffer - len);
+ memcpy(open->path, src->path, len);
+ open->path[len] = '\0';
+ len = ALIGN_UP(SIZEOF_OPEN_FILE_ACTION_S(len), sizeof(FAR void *));
+ }
+
+ ((FAR struct spawn_general_file_action_s *)buffer)->flink = NULL;
+ ((FAR struct spawn_general_file_action_s *)buffer)->action = action;
+ if (prev)
+ {
+ prev->flink = (FAR struct spawn_general_file_action_s *)buffer;
}
+
+ prev = (FAR struct spawn_general_file_action_s *)buffer;
+ buffer += len;
}
return OK;
+
+errout:
+ kmm_free((FAR void *)*copy);
+ *copy = NULL;
+ return -EFAULT;
}
/****************************************************************************
diff --git a/binfmt/binfmt_copyargv.c b/binfmt/binfmt_copyargv.c
index 7981c349a3b..bb91847f321 100644
--- a/binfmt/binfmt_copyargv.c
+++ b/binfmt/binfmt_copyargv.c
@@ -71,9 +71,12 @@
int binfmt_copyargv(FAR char * const **copy, FAR char * const *argv)
{
FAR char **argvbuf = NULL;
+ FAR const char *arg;
FAR char *ptr;
+ FAR char *end;
size_t argvsize;
size_t argsize = 0;
+ size_t len;
int nargs = 0;
int i;
@@ -81,13 +84,13 @@ int binfmt_copyargv(FAR char * const **copy, FAR char *
const *argv)
if (argv)
{
- for (i = 0; argv[i]; i++)
+ for (i = 0; (arg = argv[i]) != NULL; i++)
{
/* Increment the size of the allocation with the size of the next
* string
*/
- argsize += strlen(argv[i]) + 1;
+ argsize += strlen(arg) + 1;
nargs++;
/* This is a sanity check to prevent running away with an
@@ -121,12 +124,20 @@ int binfmt_copyargv(FAR char * const **copy, FAR char *
const *argv)
argvbuf = (FAR char **)ptr;
ptr += argvsize;
- for (i = 0; argv[i]; i++)
+ end = ptr + argsize;
+ for (i = 0; i < nargs; i++)
{
- argvbuf[i] = ptr;
- argsize = strlen(argv[i]) + 1;
- memcpy(ptr, argv[i], argsize);
- ptr += argsize;
+ arg = argv[i];
+ if (ptr == end || arg == NULL)
+ {
+ kmm_free(argvbuf);
+ return -EFAULT;
+ }
+
+ len = strnlen(arg, end - ptr - 1);
+ argvbuf[i] = memcpy(ptr, arg, len);
+ ptr[len] = '\0';
+ ptr += len + 1;
}
/* Terminate the argv[] list */
