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 2020/06/17 14:48:16 UTC
[GitHub] [incubator-nuttx] patacongo opened a new issue #1263: pthread cleanup callbacks execute in user mode.
patacongo opened a new issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263
The logic to transition from kernel to user mode for pthread_cleanup callbacks is missing. This is a security problem In the PROTECTED and KERNEL builds: We must not call the registered function in supervisor mode! See also on_exit() and atexit() callbacks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #1263: pthread cleanup callbacks execute in user mode.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-645453760
Other, related issues noted in the TODO list:
Title: ISSUES WITH atexit(), on_exit(), AND pthread_cleanup_pop()
Description: These functions execute with the following bad properties:
1. They run with interrupts disabled,
2. They run in supervisor mode (if applicable), and
3. They do not obey any setup of PIC or address
environments. Do they need to?
4. In the case of task_delete() and pthread_cancel() without
deferred cancellation, these callbacks will run on the
thread of execution and address context of the caller of
task_delete() or pthread_cancel(). That is very bad!
The fix for all of these issues it to have the callbacks
run on the caller's thread as is currently done with
signal handlers. Signals are delivered differently in
PROTECTED and KERNEL modes: The delivery involves a
signal handling trampoline function in the user address
space and two signal handlers: One to call the signal
handler trampoline in user mode (SYS_signal_handler) and
on in with the signal handler trampoline to return to
supervisor mode (SYS_signal_handler_return)
The primary difference is in the location of the signal
handling trampoline:
- In PROTECTED mode, there is on a single user space blob
with a header at the beginning of the block (at a well-
known location. There is a pointer to the signal handler
trampoline function in that header.
- In the KERNEL mode, a special process signal handler
trampoline is used at a well-known location in every
process address space (ARCH_DATA_RESERVE->ar_sigtramp).
Status: Open
Priority: Medium Low. This is an important change to some less
important interfaces. For the average user, these
functions are just fine the way they are.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #1263: atexit, on_exit, and pthread cleanup callbacks execute in Supervisor mode.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-846405029
#3626 has been merged. This resolves security issues associated with pthreads. the comments were abstracted from #3626
@no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
This, however, is more complex for several reasons:
1. Aspecial case, however, are kernel threads. Currently, they can also have on_exit() and atexit() callbacks -- but unlike tasks, these must run in supervisor mode.
2. on_exit() and atexit() processing is more entangled task_exithook(). However, I think that that entanglement is no longer necessary. I think it is an artifact from an older design. It should be okay to move those callbacks to the beginning of the exit sequence in the current design. Hmm.. is there some reason why the exit callbacks should not execute while there could be pthreads running in an SMP configuration? There are a few things to think about.
3. The task_startup() function is the same for the FLAT and PROTECTED builds, but works differently in the KERNEL build. In that case, a crt0.o file it linked at the beginning of th problem. See for example: https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/armv7-a/crt0.c
4. There are several ways that a task can exit: exit(), task_delete(), task_reset(), assert(), abort(), .... others?
_exit() might also be in that group, but since that is reserved for emergency terminations, it probably does not need to honor the on_exit() / atexit() (In fact, it is not really safe to call _exit() at all in an embedded system since it leaves files open).
There is a block diagram of the exit sequence here: https://cwiki.apache.org/confluence/display/NUTTX/Task+Exit+Sequence I am not sure if that is 100% up to date.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #1263: atexit, on_exit, and pthread cleanup callbacks execute in Supervisor mode.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-846405029
#3626 has been merged. This resolves security issues associated with pthreads. the comments were abstracted from #3626
@no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
This, however, is more complex for several reasons:
1. Aspecial case, however, are kernel threads. Currently, they can also have on_exit() and atexit() callbacks -- but unlike tasks, these must run in supervisor mode.
2. on_exit() and atexit() processing is more entangled task_exithook(). However, I think that that entanglement is no longer necessary. I think it is an artifact from an older design. It should be okay to move those callbacks to the beginning of the exit sequence in the current design. Hmm.. is there some reason why the exit callbacks should not execute while there could be pthreads running in an SMP configuration? There are a few things to think about.
3. The task_startup() function is the same for the FLAT and PROTECTED builds, but works differently in the KERNEL build. In that case, a crt0.o file it linked at the beginning of th problem. See for example: https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/armv7-a/crt0.c
4. There are several ways that a task can exit: exit(), task_delete(), task_reset(), assert(), abort(), .... others?
_exit() might also be in that group, but since that is reserved for emergency terminations, it probably does not need to honor the on_exit() / atexit() (In fact, it is not really safe to call _exit() at all in an embedded system since it leaves files open).
There is a block diagram of the exit sequence here: https://cwiki.apache.org/confluence/display/NUTTX/Task+Exit+Sequence I am not sure if that is 100% up to date.
Related #3333 #1265
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #1263: pthread cleanup callbacks execute in user mode.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-645453543
Other, related issues noted in the TODO list:
Title: ISSUES WITH atexit(), on_exit(), AND pthread_cleanup_pop()
Description: These functions execute with the following bad properties:
1. They run with interrupts disabled,
2. They run in supervisor mode (if applicable), and
3. They do not obey any setup of PIC or address
environments. Do they need to?
4. In the case of task_delete() and pthread_cancel() without
deferred cancellation, these callbacks will run on the
thread of execution and address context of the caller of
task_delete() or pthread_cancel(). That is very bad!
The fix for all of these issues it to have the callbacks
run on the caller's thread as is currently done with
signal handlers. Signals are delivered differently in
PROTECTED and KERNEL modes: The delivery involves a
signal handling trampoline function in the user address
space and two signal handlers: One to call the signal
handler trampoline in user mode (SYS_signal_handler) and
on in with the signal handler trampoline to return to
supervisor mode (SYS_signal_handler_return)
The primary difference is in the location of the signal
handling trampoline:
- In PROTECTED mode, there is on a single user space blob
with a header at the beginning of the block (at a well-
known location. There is a pointer to the signal handler
trampoline function in that header.
- In the KERNEL mode, a special process signal handler
trampoline is used at a well-known location in every
process address space (ARCH_DATA_RESERVE->ar_sigtramp).
Status: Open
Priority: Medium Low. This is an important change to some less
important interfaces. For the average user, these
functions are just fine the way they are.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #1263: atexit, on_exit, and pthread cleanup callbacks execute in Supervisor mode.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-852593796
> @no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
>
> This, however, is more complex for several reasons:
Another special case occurs to me: It does not make sense to pass the address of task_startup() in the task_create() and/or task_spawn() APIs. In PROTECTED mode, there is one special case: When the initial, start-up task is created, [nx]task_create() is called from the OS and *not* from the application space. The internal address of task_startup() is unknown to the kernel in that case since it does not appear in the kernel space link.
I think that the task_startup() will need to called through a function pointer in userspace_s. There is already a task_startup() in userspace_s (and also in the kernel space as well).
This is not an issue for KERNEL mode. In the KERNEL build, task_create() and task_spawn() do not exist. Those APIs cannot function to start processes because the process entry point lies in a different address environment and is never known from the kernel or from a different process. In the KERNEL build, all user tasks are created by running ELF or NxFLAT binaries from a file system. So task_create() is *not* called from the kernel to start the user task. Rather, the /bin/init startup program is run from a file system.
Similarly, task_create() is not called from a user process in KERNEL mode: Instead, they must use posix_spawn() or vfork() and execv() (or the non-standard exec() API) to run the program from the file system.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #1263: atexit, on_exit, and pthread cleanup callbacks execute in Supervisor mode.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-829274686
PR #3626 addresses the security issues around pthread_cleanup_pop() being called on pthread_exit().
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo removed a comment on issue #1263: pthread cleanup callbacks execute in user mode.
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-645453760
Other, related issues noted in the TODO list:
Title: ISSUES WITH atexit(), on_exit(), AND pthread_cleanup_pop()
Description: These functions execute with the following bad properties:
1. They run with interrupts disabled,
2. They run in supervisor mode (if applicable), and
3. They do not obey any setup of PIC or address
environments. Do they need to?
4. In the case of task_delete() and pthread_cancel() without
deferred cancellation, these callbacks will run on the
thread of execution and address context of the caller of
task_delete() or pthread_cancel(). That is very bad!
The fix for all of these issues it to have the callbacks
run on the caller's thread as is currently done with
signal handlers. Signals are delivered differently in
PROTECTED and KERNEL modes: The delivery involves a
signal handling trampoline function in the user address
space and two signal handlers: One to call the signal
handler trampoline in user mode (SYS_signal_handler) and
on in with the signal handler trampoline to return to
supervisor mode (SYS_signal_handler_return)
The primary difference is in the location of the signal
handling trampoline:
- In PROTECTED mode, there is on a single user space blob
with a header at the beginning of the block (at a well-
known location. There is a pointer to the signal handler
trampoline function in that header.
- In the KERNEL mode, a special process signal handler
trampoline is used at a well-known location in every
process address space (ARCH_DATA_RESERVE->ar_sigtramp).
Status: Open
Priority: Medium Low. This is an important change to some less
important interfaces. For the average user, these
functions are just fine the way they are.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on issue #1263: pthread cleanup callbacks execute in user mode.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-650806116
Related: pthread-specific data destructors are not currently implemented. They need to be and when they are, these destructors must also run in user-mode, in the context of the pthread.
This solution proposed here is to move part of pthread_exit() into user-space at libs/libc/pthread. That differes from the solution in the TODO proposed list. The TODO list solution is overly complex and not on the roadmap for a good Unix solution: One longer term objective is to move all of the pthread logic out of the kernel and into libs/libs/pthread. The pthreads interfaces are not part of the kernel in other Unix-like implementations. The kernel does provide certain necessary hooks, but the majority of the implementation of the pthread logic is implemented in libpthread, in user-space and not within the kernel. This is a model that I think that NuttX should follow as well, but things are not well structured to do that now.
Rather, I am rather here to divide pthread_exit() into two pieces: A user space function called pthread_exit() the resides in libs/lib/pthread/lib_pthread_exit.c and a kernel space functions, say nxthread_exit() that resides in sched/pthread/pthread_exit.c:
1. pthread_exit() in libs/lib/pthread/lib_pthread_exit.c would execute when the thread exits. It would only do the things that need to be done in the same context as the pthread execution environment. This would including executing all of the pthread_cleanup functions. That would would have to be migrated from sched/pthread/pthread_exit.c. It would also include the new logic to execute the the new pthread-specific data destructors. And, finally, is would all the nxthread_exit() system function.
2. nxthread_exit() would become a system call (replacing the existing system pthread_exit() system call in syscall/ and in include/sys/syscall*.h.
3. nxthread_exit() would essentially be the existing pthread exit logic sans pthread_cleanup function calls.
This sounds straight forward, but there is a complication: We also have to catch the case were the pthread main function does not call pthread_exit() but instead just returns. Currently that is handled in the function pthread_start() in sched/pthread/pthread_create.c. In FLAT mode, it just calls the pthread main function; in other modes it calls up_pthread_start() which will switch to user mode.
I have not thought through all of the details, but this would require changes like:
1. Instead of calling the pthread main function, pthread_start() would call a trampoline function in user-space. This already happens in the PROTECTED mode via the pthread_startup() function in the userspace_s structure. But not in KERNEL mode. In KERNEL mode, it uses the SYS_pthread_start system call to start the pthread main function directly.
2. pthread_start would not call pthread_exit().
3. The trampoline function in user space would simply call the pthread main function and call pthread_exit() is the pthread main function returns.
Hmm.. The kernel gets the address of the pthread trampoline from the userspace_s structure. How would the kernel know the address of a process specific pthread thread trampoline function in the KERNEL mode? I am not sure right now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #1263: atexit, on_exit, and pthread cleanup callbacks execute in Supervisor mode.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #1263:
URL: https://github.com/apache/incubator-nuttx/issues/1263#issuecomment-852593796
> @no1wudi Are you interested in finishing this job? That is, fixing on_exit() and atexit() callbacks so that they execute in user mode in all build modes? Logically this would be a very similar job: task_startup() and task exits would have to be moved to libs/libc and the callbacks would have to be re-architected so that the call back function points and parameters lie in TLS and the functions are called in some user-space task exit logic.
>
> This, however, is more complex for several reasons:
Another special case occurs to me: It does not make sense to pass the address of task_startup() in the task_create() and/or task_spawn() APIs. In PROTECTED mode, there is one special case: When the initial, start-up task is created, [nx]task_create() is called from the OS and *not* from the application space. The internal address of task_startup() is unknown to the kernel in that case since it does not appear in the kernel space link.
I think that the task_startup() will need to called through a function pointer in userspace_s. There is already a task_startup() in userspace_s (and also in the kernel space as well).
This is not an issue for KERNEL mode. In the KERNEL build, task_create() and task_spawn() do not exist. Those APIs cannot function to start processes because the process entry point lies in a different address environment and is never known from the kernel or from a different process. In the KERNEL build, all user tasks are created by running ELF or NxFLAT binaries from a file system. So task_create() is *not* called from the kernel to start the user task. Rather, the /bin/init startup program is run from a file system.
So no access to task_startup() from the kernel is ever needed from the kernel. In fact, task_startup() does not even exist in the kernel build. Rather, the functionality of task_start() is performed by the entry point __start in each process that is provided in crt0.o
Similarly, task_create() is not called from a user process in KERNEL mode: Instead, they must use posix_spawn() or vfork() and execv() (or the non-standard exec() API) to run the program from the file system.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org