| Message ID | 20210506133758.1749233-5-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: Replace alloca() by g_malloc() | expand |
On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > The ALLOCA(3) man-page mentions its "use is discouraged". > > Replace it by a g_new() call. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > bsd-user/syscall.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > index 4abff796c76..dbee0385ceb 100644 > --- a/bsd-user/syscall.c > +++ b/bsd-user/syscall.c > @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, > abi_long arg1, > case TARGET_FREEBSD_NR_writev: > { > int count = arg3; > - struct iovec *vec; > + g_autofree struct iovec *vec = g_new(struct iovec, count); > Where is this freed? Also, alloca just moves a stack pointer, where malloc has complex interactions. Are you sure that's a safe change here? Warner - vec = alloca(count * sizeof(struct iovec)); > if (lock_iovec(VERIFY_READ, vec, arg2, count, 1) < 0) > goto efault; > ret = get_errno(writev(arg1, vec, count)); > -- > 2.26.3 > >
On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com> wrote: > > > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> The ALLOCA(3) man-page mentions its "use is discouraged". >> >> Replace it by a g_new() call. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> bsd-user/syscall.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c >> index 4abff796c76..dbee0385ceb 100644 >> --- a/bsd-user/syscall.c >> +++ b/bsd-user/syscall.c >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1, >> case TARGET_FREEBSD_NR_writev: >> { >> int count = arg3; >> - struct iovec *vec; >> + g_autofree struct iovec *vec = g_new(struct iovec, count); > > > Where is this freed? g_autofree, so it gets freed when it goes out of scope. https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > Also, alloca just moves a stack pointer, where malloc has complex interactions. Are you sure that's a safe change here? alloca()ing something with size determined by the guest is definitely not safe :-) malloc as part of "handle this syscall" is pretty common, at least in linux-user. thanks -- PMM
On 5/6/21 9:16 AM, Warner Losh wrote: > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> > wrote: > >> The ALLOCA(3) man-page mentions its "use is discouraged". >> >> Replace it by a g_new() call. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> bsd-user/syscall.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c >> index 4abff796c76..dbee0385ceb 100644 >> --- a/bsd-user/syscall.c >> +++ b/bsd-user/syscall.c >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, >> abi_long arg1, >> case TARGET_FREEBSD_NR_writev: >> { >> int count = arg3; >> - struct iovec *vec; >> + g_autofree struct iovec *vec = g_new(struct iovec, count); >> > > Where is this freed? Also, alloca just moves a stack pointer, where malloc > has complex interactions. Are you sure that's a safe change here? It's freed any time the g_autofree variable goes out of scope (that's what the g_autofree macro is for). Yes, the change is safe, although you are right that switching to malloc is going to be a bit more heavyweight than what alloca used. What's more, it adds safety: if count was under user control, a user could pass a value that could cause alloca to allocate more than 4k and accidentally mess up stack guard pages, while malloc() uses the heap and therefore cannot cause stack bugs.
On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com> wrote: > > > > > > > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> > wrote: > >> > >> The ALLOCA(3) man-page mentions its "use is discouraged". > >> > >> Replace it by a g_new() call. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> bsd-user/syscall.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > >> index 4abff796c76..dbee0385ceb 100644 > >> --- a/bsd-user/syscall.c > >> +++ b/bsd-user/syscall.c > >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, > abi_long arg1, > >> case TARGET_FREEBSD_NR_writev: > >> { > >> int count = arg3; > >> - struct iovec *vec; > >> + g_autofree struct iovec *vec = g_new(struct iovec, count); > > > > > > Where is this freed? > > g_autofree, so it gets freed when it goes out of scope. > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree Ah. I'd missed that feature and annotation... Too many years seeing patches like this in other projects where a feature like this wasn't there to save the day... This means you can ignore my other message as equally misinformed. > > > Also, alloca just moves a stack pointer, where malloc has complex > interactions. Are you sure that's a safe change here? > > alloca()ing something with size determined by the guest is > definitely not safe :-) malloc as part of "handle this syscall" > is pretty common, at least in linux-user. > Well, since this is userland emulation, the normal process boundaries will fix that. allocating from the heap is little different..., so while unsafe, it's the domain of $SOMEONE_ELSE to enforce the safety. linux-user has many similar usages for both malloc and alloca, and it's ok for the same reason. Doing a quick grep on my blitz[*] branch in the bsd-user fork shows that alloca is used almost exclusively there. There's 24 calls to alloca in the bsd-user code. There's almost no malloc calls (7) in that at all outside the image loader: all but one of them are confined to sysctl emulation with the last one used to keep track of thread state in new threads... linux-user has a similar profile (20 alloca's and 14 mallocs outside the loader), but with more mallocs in other paths than just sysctl (which isn't present). I had no plans on migrating to anything else... Warner
On Thu, May 6, 2021 at 8:26 AM Eric Blake <eblake@redhat.com> wrote: > On 5/6/21 9:16 AM, Warner Losh wrote: > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> > > wrote: > > > >> The ALLOCA(3) man-page mentions its "use is discouraged". > >> > >> Replace it by a g_new() call. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> bsd-user/syscall.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > >> index 4abff796c76..dbee0385ceb 100644 > >> --- a/bsd-user/syscall.c > >> +++ b/bsd-user/syscall.c > >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, > >> abi_long arg1, > >> case TARGET_FREEBSD_NR_writev: > >> { > >> int count = arg3; > >> - struct iovec *vec; > >> + g_autofree struct iovec *vec = g_new(struct iovec, count); > >> > > > > Where is this freed? Also, alloca just moves a stack pointer, where > malloc > > has complex interactions. Are you sure that's a safe change here? > > It's freed any time the g_autofree variable goes out of scope (that's > what the g_autofree macro is for). Yes, the change is safe, although > you are right that switching to malloc is going to be a bit more > heavyweight than what alloca used. What's more, it adds safety: if > count was under user control, a user could pass a value that could cause > alloca to allocate more than 4k and accidentally mess up stack guard > pages, while malloc() uses the heap and therefore cannot cause stack bugs. > I'm not sure I understand that argument, since we're not compiling bsd-user with the stack-smash-protection stuff enabled, so there's no guard pages involved. The stack can grow quite large and the unmapped page at the end of the stack would catch any overflows. Since these allocations are on the top of the stack, they won't overflow into any other frames and subsequent calls are made with them already in place. malloc, on the other hand, involves taking out a number of mutexes and similar things to obtain the memory, which may not necessarily be safe in all the contexts system calls can be called from. System calls are, typically, async safe and can be called from signal handlers. alloca() is async safe, while malloc() is not. So changing the calls from alloca to malloc makes calls to system calls in signal handlers unsafe and potentially introducing buggy behavior as a result. Warner
On 5/6/21 4:48 PM, Warner Losh wrote: > > > On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org > <mailto:peter.maydell@linaro.org>> wrote: > > On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com > <mailto:imp@bsdimp.com>> wrote: > > > > > > > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote: > >> > >> The ALLOCA(3) man-page mentions its "use is discouraged". > >> > >> Replace it by a g_new() call. > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> > >> --- > >> bsd-user/syscall.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > >> index 4abff796c76..dbee0385ceb 100644 > >> --- a/bsd-user/syscall.c > >> +++ b/bsd-user/syscall.c > >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, > int num, abi_long arg1, > >> case TARGET_FREEBSD_NR_writev: > >> { > >> int count = arg3; > >> - struct iovec *vec; > >> + g_autofree struct iovec *vec = g_new(struct iovec, > count); > > > > > > Where is this freed? > > g_autofree, so it gets freed when it goes out of scope. > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree> > > > Ah. I'd missed that feature and annotation... Too many years seeing > patches like > this in other projects where a feature like this wasn't there to save > the day... > > This means you can ignore my other message as equally misinformed. This also shows my commit description is poor. > > Also, alloca just moves a stack pointer, where malloc has complex > interactions. Are you sure that's a safe change here? > > alloca()ing something with size determined by the guest is > definitely not safe :-) malloc as part of "handle this syscall" > is pretty common, at least in linux-user. > > > Well, since this is userland emulation, the normal process boundaries > will fix that. allocating from > the heap is little different..., so while unsafe, it's the domain of > $SOMEONE_ELSE to enforce > the safety. linux-user has many similar usages for both malloc and > alloca, and it's ok for the > same reason. > > Doing a quick grep on my blitz[*] branch in the bsd-user fork shows that > alloca is used almost > exclusively there. There's 24 calls to alloca in the bsd-user code. > There's almost no malloc > calls (7) in that at all outside the image loader: all but one of them > are confined to sysctl > emulation with the last one used to keep track of thread state in new > threads... > linux-user has a similar profile (20 alloca's and 14 mallocs outside the > loader), > but with more mallocs in other paths than just sysctl (which isn't present). > > I had no plans on migrating to anything else... I considered excluding user emulation (both Linux/BSD) by enabling CFLAGS=-Walloca for everything except user-mode, but Meson doesn't support adding flags to a specific source set: https://github.com/mesonbuild/meson/issues/1367#issuecomment-277929767 Q: Is it possible to add a flag to a specific file? I have some generated code that's freaking the compiler out and I don't feel like mucking with the generator. A: We don't support per-file compiler flags by design. It interacts very poorly with other parts, especially precompiled headers. The real solution is to fix the generator to not produce garbage. Obviously this is often not possible so the solution to that is, as mentioned above, build a static library with the specific compiler args. This has the added benefit that it makes this workaround explicit and visible rather than hiding things in random locations. Then Paolo suggested to convert all alloca() calls instead. Regards, Phil.
On Thu, May 6, 2021 at 8:57 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 5/6/21 4:48 PM, Warner Losh wrote: > > > > > > On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org > > <mailto:peter.maydell@linaro.org>> wrote: > > > > On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com > > <mailto:imp@bsdimp.com>> wrote: > > > > > > > > > > > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daudé > > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote: > > >> > > >> The ALLOCA(3) man-page mentions its "use is discouraged". > > >> > > >> Replace it by a g_new() call. > > >> > > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com > > <mailto:philmd@redhat.com>> > > >> --- > > >> bsd-user/syscall.c | 3 +-- > > >> 1 file changed, 1 insertion(+), 2 deletions(-) > > >> > > >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > > >> index 4abff796c76..dbee0385ceb 100644 > > >> --- a/bsd-user/syscall.c > > >> +++ b/bsd-user/syscall.c > > >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, > > int num, abi_long arg1, > > >> case TARGET_FREEBSD_NR_writev: > > >> { > > >> int count = arg3; > > >> - struct iovec *vec; > > >> + g_autofree struct iovec *vec = g_new(struct iovec, > > count); > > > > > > > > > Where is this freed? > > > > g_autofree, so it gets freed when it goes out of scope. > > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > > < > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree > > > > > > > > Ah. I'd missed that feature and annotation... Too many years seeing > > patches like > > this in other projects where a feature like this wasn't there to save > > the day... > > > > This means you can ignore my other message as equally misinformed. > > This also shows my commit description is poor. > > > > Also, alloca just moves a stack pointer, where malloc has complex > > interactions. Are you sure that's a safe change here? > > > > alloca()ing something with size determined by the guest is > > definitely not safe :-) malloc as part of "handle this syscall" > > is pretty common, at least in linux-user. > > > > > > Well, since this is userland emulation, the normal process boundaries > > will fix that. allocating from > > the heap is little different..., so while unsafe, it's the domain of > > $SOMEONE_ELSE to enforce > > the safety. linux-user has many similar usages for both malloc and > > alloca, and it's ok for the > > same reason. > > > > Doing a quick grep on my blitz[*] branch in the bsd-user fork shows that > > alloca is used almost > > exclusively there. There's 24 calls to alloca in the bsd-user code. > > There's almost no malloc > > calls (7) in that at all outside the image loader: all but one of them > > are confined to sysctl > > emulation with the last one used to keep track of thread state in new > > threads... > > linux-user has a similar profile (20 alloca's and 14 mallocs outside the > > loader), > > but with more mallocs in other paths than just sysctl (which isn't > present). > > > > I had no plans on migrating to anything else... > > I considered excluding user emulation (both Linux/BSD) by enabling > CFLAGS=-Walloca for everything except user-mode, but Meson doesn't > support adding flags to a specific source set: > https://github.com/mesonbuild/meson/issues/1367#issuecomment-277929767 > > Q: Is it possible to add a flag to a specific file? I have some > generated code that's freaking the compiler out and I don't > feel like mucking with the generator. > > A: We don't support per-file compiler flags by design. It interacts > very poorly with other parts, especially precompiled headers. > The real solution is to fix the generator to not produce garbage. > Obviously this is often not possible so the solution to that is, > as mentioned above, build a static library with the specific > compiler args. This has the added benefit that it makes this > workaround explicit and visible rather than hiding things in > random locations. > > Then Paolo suggested to convert all alloca() calls instead. > I'm having trouble understanding how the emulated system calls can still be async safe if that's done. I might be missing something in the CPU emulation that would clear up the concern, though. How threads interact with each-other in emulation is an area I'm still learning. Warner P.S. Of course, no matter what you do, the current in-tree bsd-user dumps core right away, so it's hard to break it worse than it already is broken :)...
On 5/6/21 9:55 AM, Warner Losh wrote: >>> Where is this freed? Also, alloca just moves a stack pointer, where >> malloc >>> has complex interactions. Are you sure that's a safe change here? >> >> It's freed any time the g_autofree variable goes out of scope (that's >> what the g_autofree macro is for). Yes, the change is safe, although >> you are right that switching to malloc is going to be a bit more >> heavyweight than what alloca used. What's more, it adds safety: if >> count was under user control, a user could pass a value that could cause >> alloca to allocate more than 4k and accidentally mess up stack guard >> pages, while malloc() uses the heap and therefore cannot cause stack bugs. >> > > I'm not sure I understand that argument, since we're not compiling bsd-user > with the stack-smash-protection stuff enabled, so there's no guard pages > involved. The stack can grow quite large and the unmapped page at > the end of the stack would catch any overflows. Since these allocations > are on the top of the stack, they won't overflow into any other frames > and subsequent calls are made with them already in place. With alloca() on user-controlled size, the user can set up the size to be larger than the unmapped guard page, at which point you CANNOT catch the stack overflow because the alloca can skip the guard page and wrap into other valid memory. Compiling with stack-smash-protection stuff enabled will catch such a bad alloca(); but the issue at hand here is not when stack-smash-protection is enabled, but the more common case when it is disabled (at which point the only protection you have is the guard page, but improper use of alloca() can bypass the guard page). Not all alloca() arguments are under user control, but it is easier as a matter of policy to blindly avoid alloca() than it is to audit which calls have safe sizes and therefore will not risk user control bypassing stack guards. > > malloc, on the other hand, involves taking out a number of mutexes > and similar things to obtain the memory, which may not necessarily > be safe in all the contexts system calls can be called from. System > calls are, typically, async safe and can be called from signal handlers. > alloca() is async safe, while malloc() is not. So changing the calls > from alloca to malloc makes calls to system calls in signal handlers > unsafe and potentially introducing buggy behavior as a result. Correct, use of malloc() is not safe within signal handlers. But these calls are not within signal handlers - or am _I_ missing something? Is the point of *-user code to emulate syscalls that are reachable from code installed in a signal handler, at which point introducing an async-unsafe call to malloc in our emulation is indeed putting the application at risk of a malloc deadlock? Ultimately, we're trading one maintenance headache (determining which alloca() size calls might be under user control) for another (determining that malloca() calls are not in a signal context), but the latter is far more common such that we can use existing tooling to make that conversion safely (both in the fact that the compiler has flags to warn about alloca() usage, and in the fact that Coverity is good at flagging improper uses of malloc() such as within a function reachable from something installed in a signal handler). But I'm not familiar enough with the bsd/linux-user code to know if your point about having to use only async-safe functionalities is a valid limitation on our emulation.
On Thu, May 6, 2021 at 9:12 AM Eric Blake <eblake@redhat.com> wrote: > On 5/6/21 9:55 AM, Warner Losh wrote: > > >>> Where is this freed? Also, alloca just moves a stack pointer, where > >> malloc > >>> has complex interactions. Are you sure that's a safe change here? > >> > >> It's freed any time the g_autofree variable goes out of scope (that's > >> what the g_autofree macro is for). Yes, the change is safe, although > >> you are right that switching to malloc is going to be a bit more > >> heavyweight than what alloca used. What's more, it adds safety: if > >> count was under user control, a user could pass a value that could cause > >> alloca to allocate more than 4k and accidentally mess up stack guard > >> pages, while malloc() uses the heap and therefore cannot cause stack > bugs. > >> > > > > I'm not sure I understand that argument, since we're not compiling > bsd-user > > with the stack-smash-protection stuff enabled, so there's no guard pages > > involved. The stack can grow quite large and the unmapped page at > > the end of the stack would catch any overflows. Since these allocations > > are on the top of the stack, they won't overflow into any other frames > > and subsequent calls are made with them already in place. > > With alloca() on user-controlled size, the user can set up the size to > be larger than the unmapped guard page, at which point you CANNOT catch > the stack overflow because the alloca can skip the guard page and wrap > into other valid memory. Compiling with stack-smash-protection stuff > enabled will catch such a bad alloca(); but the issue at hand here is > not when stack-smash-protection is enabled, but the more common case > when it is disabled (at which point the only protection you have is the > guard page, but improper use of alloca() can bypass the guard page). > Not all alloca() arguments are under user control, but it is easier as a > matter of policy to blindly avoid alloca() than it is to audit which > calls have safe sizes and therefore will not risk user control bypassing > stack guards. > I'd forgotten that the gap pages are only a single page and things may be mapped beyond that. For bsd-user, though, any issues that this cause will be mitigated by the fact that it's running as the user that we're emulating, so there's no privilege escalations that couldn't already be hand-coded w/o bsd-user in the loop. > > > > malloc, on the other hand, involves taking out a number of mutexes > > and similar things to obtain the memory, which may not necessarily > > be safe in all the contexts system calls can be called from. System > > calls are, typically, async safe and can be called from signal handlers. > > alloca() is async safe, while malloc() is not. So changing the calls > > from alloca to malloc makes calls to system calls in signal handlers > > unsafe and potentially introducing buggy behavior as a result. > > Correct, use of malloc() is not safe within signal handlers. But these > calls are not within signal handlers - or am _I_ missing something? Is > the point of *-user code to emulate syscalls that are reachable from > code installed in a signal handler, at which point introducing an > async-unsafe call to malloc in our emulation is indeed putting the > application at risk of a malloc deadlock? > My concern is that the user emulation has some elements that are based on catching signals and interpreting them to control execution in the guest code. There's a number of places where signals are queued instead of run right away as well. I've not had time to dive into the complex implementation to make sure that we'll not wind up in a situation where we would be trying to call malloc from a signal handler directly. I'll try to find an answer to that as well. > Ultimately, we're trading one maintenance headache (determining which > alloca() size calls might be under user control) for another > (determining that malloca() calls are not in a signal context), but the > latter is far more common such that we can use existing tooling to make > that conversion safely (both in the fact that the compiler has flags to > warn about alloca() usage, and in the fact that Coverity is good at > flagging improper uses of malloc() such as within a function reachable > from something installed in a signal handler). But I'm not familiar > enough with the bsd/linux-user code to know if your point about having > to use only async-safe functionalities is a valid limitation on our > emulation. > Yea, my faith in Coverity flagging these when there's trips between, say, arm binary code and user code isn't quite as high as yours. But for the real answer, I need to contact the original authors of this part of the code (they are no longer involved day-to-day in the bsd-user efforts) to see if this scenario is possible or not. If it's easy to find out that way, we can either know this is safe to do, or if effort is needed to make it safe. At present, I've seen enough and chatted enough with others to be concerned that the change would break proper emulation. Warner
On 5/6/21 10:30 AM, Warner Losh wrote: > > But for the real answer, I need to contact the original authors of > this part of the code (they are no longer involved day-to-day in > the bsd-user efforts) to see if this scenario is possible or not. If > it's easy to find out that way, we can either know this is safe to > do, or if effort is needed to make it safe. At present, I've seen > enough and chatted enough with others to be concerned that > the change would break proper emulation. Do we have a feel for the maximum amount of memory being used by the various alloca() replaced in this series? If so, can we just stack-allocate an array of bytes of the maximum size needed? Then we avoid alloca() but also avoid the dynamic memory management that malloc() would introduce. Basically, it boils down to auditing why the alloca() is safe, and once we know that, replacing the variable-sized precise alloca() with its counterpart statically-sized array allocation, at the expense of some wasted stack space when the runtime size does not use the full compile-time maximum size.
On Thu, 6 May 2021 at 15:57, Warner Losh <imp@bsdimp.com> wrote: > malloc, on the other hand, involves taking out a number of mutexes > and similar things to obtain the memory, which may not necessarily > be safe in all the contexts system calls can be called from. System > calls are, typically, async safe and can be called from signal handlers. > alloca() is async safe, while malloc() is not. So changing the calls > from alloca to malloc makes calls to system calls in signal handlers > unsafe and potentially introducing buggy behavior as a result. malloc() should definitely be fine in this context. The syscall emulation is called after the cpu_loop() in bsd-user has called cpu_exec(). cpu_exec() calls into the JIT, which will malloc all over the place if it needs to in the course of JITting things. This code should never be being called from a (host) signal handler. In upstream the signal handling code for bsd-user appears to be missing entirely. For linux-user when we take a host signal we merely arrange for the guest to take a guest signal, we don't try to execute guest code directly from the host handler. (There are some pretty hairy races in emulated signal handling: we did a big overhaul of the linux-user code in that area a while back. If your bsd-user code doesn't have the 'safe_syscall' stuff it probably needs it. But that's more about races between "guest code wants to execute a syscall" and "incoming signal" where we could fail to exit EINTR from an emulated syscall if the signal arrives in a bad window.) thanks -- PMM
On Thu, 6 May 2021 at 16:46, Eric Blake <eblake@redhat.com> wrote: > > On 5/6/21 10:30 AM, Warner Losh wrote: > > > > > But for the real answer, I need to contact the original authors of > > this part of the code (they are no longer involved day-to-day in > > the bsd-user efforts) to see if this scenario is possible or not. If > > it's easy to find out that way, we can either know this is safe to > > do, or if effort is needed to make it safe. At present, I've seen > > enough and chatted enough with others to be concerned that > > the change would break proper emulation. > > Do we have a feel for the maximum amount of memory being used by the > various alloca() replaced in this series? If so, can we just > stack-allocate an array of bytes of the maximum size needed? In *-user the allocas are generally of the form "guest passed us a random number, allocate that many structs/whatevers". (In this specific bsd-user example it's the writev syscall and it's "however many struct iovecs the guest passed".) So there is no upper limit. The right thing to do here is probably to use g_try_malloc() and return ENOMEM or whatever on failure. The use of alloca, at least in the linux-user code, is purely old lazy coding based on "in practice real world guest binaries don't allocate very many of these so we can get away with shoving them on the stack". thanks -- PMM
On Thu, May 6, 2021 at 9:59 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 6 May 2021 at 15:57, Warner Losh <imp@bsdimp.com> wrote: > > malloc, on the other hand, involves taking out a number of mutexes > > and similar things to obtain the memory, which may not necessarily > > be safe in all the contexts system calls can be called from. System > > calls are, typically, async safe and can be called from signal handlers. > > alloca() is async safe, while malloc() is not. So changing the calls > > from alloca to malloc makes calls to system calls in signal handlers > > unsafe and potentially introducing buggy behavior as a result. > > malloc() should definitely be fine in this context. The syscall > emulation is called after the cpu_loop() in bsd-user has called > cpu_exec(). cpu_exec() calls into the JIT, which will malloc > all over the place if it needs to in the course of JITting things. > Gotcha. That's the context I was trying to chase down via other vectors. > This code should never be being called from a (host) signal handler. > In upstream the signal handling code for bsd-user appears to > be missing entirely. For linux-user when we take a host signal > we merely arrange for the guest to take a guest signal, we > don't try to execute guest code directly from the host handler. > OK. Then that makes sense to me now. I'll look through the bsd-user stuff in our branch... > (There are some pretty hairy races in emulated signal handling: > we did a big overhaul of the linux-user code in that area a > while back. If your bsd-user code doesn't have the 'safe_syscall' > stuff it probably needs it. But that's more about races between > "guest code wants to execute a syscall" and "incoming signal" > where we could fail to exit EINTR from an emulated syscall if > the signal arrives in a bad window.) > Those have not yet been moved over entirely. And it was looking through those patches that gave me a health respect (fear?) of issues with mixing of host/guest signals... Warner
On Thu, May 6, 2021 at 10:04 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 6 May 2021 at 16:46, Eric Blake <eblake@redhat.com> wrote: > > > > On 5/6/21 10:30 AM, Warner Losh wrote: > > > > > > > > But for the real answer, I need to contact the original authors of > > > this part of the code (they are no longer involved day-to-day in > > > the bsd-user efforts) to see if this scenario is possible or not. If > > > it's easy to find out that way, we can either know this is safe to > > > do, or if effort is needed to make it safe. At present, I've seen > > > enough and chatted enough with others to be concerned that > > > the change would break proper emulation. > > > > Do we have a feel for the maximum amount of memory being used by the > > various alloca() replaced in this series? If so, can we just > > stack-allocate an array of bytes of the maximum size needed? > > In *-user the allocas are generally of the form "guest passed > us a random number, allocate that many structs/whatevers". (In this > specific bsd-user example it's the writev syscall and it's "however > many struct iovecs the guest passed".) So there is no upper limit. > > The right thing to do here is probably to use g_try_malloc() and return > ENOMEM or whatever on failure. The use of alloca, at least in the > linux-user code, is purely old lazy coding based on "in practice > real world guest binaries don't allocate very many of these so > we can get away with shoving them on the stack". > I'll convert the ones in our fork to use that pattern prior to upstreaming. Warner
diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c index 4abff796c76..dbee0385ceb 100644 --- a/bsd-user/syscall.c +++ b/bsd-user/syscall.c @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1, case TARGET_FREEBSD_NR_writev: { int count = arg3; - struct iovec *vec; + g_autofree struct iovec *vec = g_new(struct iovec, count); - vec = alloca(count * sizeof(struct iovec)); if (lock_iovec(VERIFY_READ, vec, arg2, count, 1) < 0) goto efault; ret = get_errno(writev(arg1, vec, count));
The ALLOCA(3) man-page mentions its "use is discouraged". Replace it by a g_new() call. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- bsd-user/syscall.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)