You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/05/02 12:23:17 UTC
[GitHub] [incubator-nuttx] pussuw opened a new pull request, #6193: Elf loader: give temporary write access to user .text memory
pussuw opened a new pull request, #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193
When the .elf file is loaded from disk, the kernel must be given write
access to the allocated .text section in the task's address environment.
The access is removed after the elf is loaded and relocations are done.
NOTE:
The reason this works for the ARM implementation, is that the ARM MMU
can be configured to give write access for the privileged mode, but
revoke write access for the user mode.
Regardless, it would be smart to revoke write access even for the
kernel, when the kernel does not need it. This framework allows doing
that, if someone wishes to take up the task.
## Summary
## Impact
## Testing
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193#discussion_r863726147
##########
include/nuttx/arch.h:
##########
@@ -1185,6 +1185,44 @@ int up_addrenv_attach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
int up_addrenv_detach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
#endif
+/****************************************************************************
+ * Name: up_addrenv_text_enable_write
+ *
+ * Description:
+ * Temporarily enable write access to the .text section. This must be
+ * called prior to loading the process code into memory.
+ *
+ * Input Parameters:
+ * addrenv - The address environment to be modified.
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_ADDRENV
+int up_addrenv_text_enable_write(FAR group_addrenv_t *addrenv);
Review Comment:
Since it's very useful to modify the permission of a memory region in other case(e.g. mprotect/mmap). Could you generalize arch like this(https://man7.org/linux/man-pages/man2/mprotect.2.html):
```
int up_addrenv_protect(FAR group_addrenv_t *addrenv, FAR void *addr, size_t len, int prot);
```
and call this function from binfmt instead.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193#discussion_r863726147
##########
include/nuttx/arch.h:
##########
@@ -1185,6 +1185,44 @@ int up_addrenv_attach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
int up_addrenv_detach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
#endif
+/****************************************************************************
+ * Name: up_addrenv_text_enable_write
+ *
+ * Description:
+ * Temporarily enable write access to the .text section. This must be
+ * called prior to loading the process code into memory.
+ *
+ * Input Parameters:
+ * addrenv - The address environment to be modified.
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_ADDRENV
+int up_addrenv_text_enable_write(FAR group_addrenv_t *addrenv);
Review Comment:
Since it's very useful to modify the permission of a memory region in other case(e.g. mprotect/mmap). Could you generalize arch api like this(https://man7.org/linux/man-pages/man2/mprotect.2.html):
```
int up_addrenv_protect(FAR group_addrenv_t *addrenv, FAR void *addr, size_t len, int prot);
```
and call this function from binfmt instead.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193#discussion_r863947997
##########
include/nuttx/arch.h:
##########
@@ -1185,6 +1185,44 @@ int up_addrenv_attach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
int up_addrenv_detach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
#endif
+/****************************************************************************
+ * Name: up_addrenv_text_enable_write
+ *
+ * Description:
+ * Temporarily enable write access to the .text section. This must be
+ * called prior to loading the process code into memory.
+ *
+ * Input Parameters:
+ * addrenv - The address environment to be modified.
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_ADDRENV
+int up_addrenv_text_enable_write(FAR group_addrenv_t *addrenv);
Review Comment:
I thought of making a more generic API for modifying access rights but I currently only need to modify the .text region so to keep things simple I did only that.
I can do the generalization later since it does make sense, but I don't have time to do it now.I think we will need it ourselves when mmap is taken into use, but I need C++ support for kernel mode first.
Basically the infrastructure is already here, just need to modify this prototype and declare generic "PROT_WR/RD/EX" flags in some common header, maybe even here.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193#discussion_r863722914
##########
binfmt/libelf/libelf_addrenv.c:
##########
@@ -145,6 +145,92 @@ int elf_addrenv_alloc(FAR struct elf_loadinfo_s *loadinfo, size_t textsize,
#endif
}
+/****************************************************************************
+ * Name: elf_addrenv_restore
+ *
+ * Description:
+ * Restore the address environment before elf_addrenv_select() was called..
+ *
+ * Input Parameters:
+ * loadinfo - Load state information
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_ADDRENV
+int elf_addrenv_select(FAR struct elf_loadinfo_s *loadinfo)
+{
+ int ret;
+
+ /* Instantiate the new address environment */
+
+ ret = up_addrenv_select(&loadinfo->addrenv, &loadinfo->oldenv);
+ if (ret < 0)
+ {
+ berr("ERROR: up_addrenv_select failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Allow write access to .text */
+
+ ret = up_addrenv_text_enable_write(&loadinfo->addrenv);
+ if (ret < 0)
+ {
+ berr("ERROR: up_addrenv_text_enable_write failed: %d\n", ret);
+ return ret;
+ }
+
+ return OK;
+}
+#endif
+
+/****************************************************************************
+ * Name: elf_addrenv_free
Review Comment:
correct the comment
##########
binfmt/libelf/libelf_addrenv.c:
##########
@@ -145,6 +145,92 @@ int elf_addrenv_alloc(FAR struct elf_loadinfo_s *loadinfo, size_t textsize,
#endif
}
+/****************************************************************************
+ * Name: elf_addrenv_restore
Review Comment:
correct comment too
##########
include/nuttx/arch.h:
##########
@@ -1185,6 +1185,44 @@ int up_addrenv_attach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
int up_addrenv_detach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
#endif
+/****************************************************************************
+ * Name: up_addrenv_text_enable_write
+ *
+ * Description:
+ * Temporarily enable write access to the .text section. This must be
+ * called prior to loading the process code into memory.
+ *
+ * Input Parameters:
+ * addrenv - The address environment to be modified.
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_ADDRENV
+int up_addrenv_text_enable_write(FAR group_addrenv_t *addrenv);
Review Comment:
Since it's very useful to modify the permission for a memory region in other case(e.g. mprotect/mmap). Could you generalize arch like this(https://man7.org/linux/man-pages/man2/mprotect.2.html):
```
int up_addrenv_protect(FAR group_addrenv_t *addrenv, FAR void *addr, size_t len, int prot);
```
and call this function from binfmt instead.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193#discussion_r863943105
##########
binfmt/libelf/libelf_addrenv.c:
##########
@@ -145,6 +145,92 @@ int elf_addrenv_alloc(FAR struct elf_loadinfo_s *loadinfo, size_t textsize,
#endif
}
+/****************************************************************************
+ * Name: elf_addrenv_restore
Review Comment:
Silly copy&paste error, I'll make a patch of it
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 merged pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
masayuki2009 merged PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6193: Elf loader: give temporary write access to user .text memory
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6193:
URL: https://github.com/apache/incubator-nuttx/pull/6193#discussion_r863947997
##########
include/nuttx/arch.h:
##########
@@ -1185,6 +1185,44 @@ int up_addrenv_attach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
int up_addrenv_detach(FAR struct task_group_s *group, FAR struct tcb_s *tcb);
#endif
+/****************************************************************************
+ * Name: up_addrenv_text_enable_write
+ *
+ * Description:
+ * Temporarily enable write access to the .text section. This must be
+ * called prior to loading the process code into memory.
+ *
+ * Input Parameters:
+ * addrenv - The address environment to be modified.
+ *
+ * Returned Value:
+ * Zero (OK) on success; a negated errno value on failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_ARCH_ADDRENV
+int up_addrenv_text_enable_write(FAR group_addrenv_t *addrenv);
Review Comment:
I thought of making a more generic API for modifying access rights but I currently only need to modify the .text region so to keep things simple I did only that.
I can do the generalization later since it does make sense, but I don't have time to do it now. I thionk we will need it ourselves when mmap is taken into use.
Basically the infrastructure is already here, just need to modify this prototype and declare generic "PROT_WR/RD/EX" flags in some common header, maybe even here.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org