Re: less: fix use after free bug
On Fri, Dec 31, 2021 at 6:22 AM Tobias Stoeckmann wrote: > Hi, > > it is possible to trigger a use after free bug in less with huge > files or tight memory constraints. PoC with 100 MB file: > > dd if=/dev/zero bs=1024 count=102400 | tr '\0' 'a' > less-poc.txt > ulimit -d 157286 > less less-poc.txt > > The linebuf and attr buffers in line.c are supposed to never be freed, > since they are used for keeping (meta) data of current line in RAM. > > The linebuf and attr buffers are expanded in expand_linebuf. If linebuf > could be expanded but attr could not, then returned pointers are freed, > but linebuf variable still points to previous location. Subsequent > accesses will therefore trigger a use after free bug. > > The easiest fix is to keep reallocated linebuf. The size of both buffers > differ, but the code still assumes that the previous (smaller) size is > the correct one. > > Thoughts on this approach? > Analysis makes sense. To bikeshed slightly I would be inclined to do the work progressively, perhaps like the diff below...but your diff works too. Philip Guenther Index: line.c === RCS file: /data/src/openbsd/src/usr.bin/less/line.c,v retrieving revision 1.34 diff -u -p -r1.34 line.c --- line.c 25 Oct 2021 19:54:29 - 1.34 +++ line.c 1 Jan 2022 06:24:31 - @@ -96,16 +96,16 @@ expand_linebuf(void) /* Just realloc to expand the buffer, if we can. */ char *new_buf = recallocarray(linebuf, size_linebuf, new_size, 1); - char *new_attr = recallocarray(attr, size_linebuf, new_size, 1); - if (new_buf == NULL || new_attr == NULL) { - free(new_attr); - free(new_buf); - return (1); + if (new_buf != NULL) { + char *new_attr = recallocarray(attr, size_linebuf, new_size, 1); + linebuf = new_buf; + if (new_attr != NULL) { + attr = new_attr; + size_linebuf = new_size; + return (0); + } } - linebuf = new_buf; - attr = new_attr; - size_linebuf = new_size; - return (0); + return (1); } /*
Re: Fix GNUism in bsd.dep.mk
On Fri, Dec 31, 2021 at 7:44 AM Christian Ehrhardt wrote: > Here at genua, trying to build libpcap sometimes breaks in > libpcap with the following error message: > > | Using $< in a non-suffix rule context is a GNUmake idiom \ > |(/data/git/ehrhardt/genuos/os/mk/bsd.dep.mk:47) > > The bug is in bsd.dep.mk where ${.IMPSRC} (aka $<) is used > in a context where it is not neccessarily defined by OpenBSD make. > > Below is a diff to Use ${.ALLSRC:M*.y} instead of ${.IMPSRC} as > the input file for yacc. > > The issue is with the rule for the grammar.h file that is generated > by yacc from grammar.c. You can easily reproduce the bug with the > following steps: > - build libpcap from scratch: cd .../src/lib/libpcap && make clean all > - remove the generated grammar.h file: rm obj*/grammar.h > - build libpcap again (incremental build): make > In normal builds this does not trigger as grammar.h is implicitly > generated by the rule for grammar.c and when make checks for > dependencies it simply finds grammar.h uptodate. However, > incremental or parallel builds might decide to make grammar.h > from grammar.y. > > Now, why is this only a problem for grammar.h but not for > grammar.c? The answer to this question is burried deeply in > OpenBSD's mk files. > > The snippet in bsd.dep.mk that triggers the error is a single > rule statement that generates foo.c and foo.h from foo.y with a > call to yacc -d. The rule is generated with a loop, i.e. it is not > a prefix rule. However, a prefix rule context is required for > the use of ${.IMPSRC} aka $<. For the .c file such a prefix > rule is provided by bsd.sys.mk and this rule is in scope when > make evaluates the yacc rule. However, for .h file generation > from a .y file there is no such prefix rule defined in any of > the Makefiles. Even if it were the .h suffix is missing from > .SUFFIXES and the rule would not be considered. > > NOTE: The obvious way to fix this would be to use $f instead of > ${.IMPSRC}. However, this does not work as $f is then missing > the path prefix and yacc won't find it if an obj directory is > used. This is probably the reason for the use of ${.IMPSRC} in > the first place. > Ah, nice analysis! The suggested fix looks safe to me (can't see how a .c could have two .y direct prerequisites, so this can't be ambiguous). ok guenther@
Re: unlock mmap(2) for anonymous mappings
The uvm_wxabort path within uvm_wxcheck() looks not MP-safe. > On 31 Dec 2021, at 12:14, Klemens Nanni wrote: > > Now that mpi has unlocked uvm's fault handler, we can unlock the mmap > syscall to handle MAP_ANON without the big lock. > > sys_mmap() still protects the !MAP_ANON case, i.e. file based mappings, > with the KERNEL_LOCK() itself, which is why unlocking the syscall will > only change locking behaviour for anonymous mappings. > > A previous to unlock file based mappings was reverted, see the following > from https://marc.info/?l=openbsd-tech&m=160155434212888&w=2 : > > commit 38802bc07455f2a4f8cdde272850a5eab5dfa6c8 > from: mpi > date: Wed Oct 7 12:26:20 2020 UTC > > Do not release the KERNEL_LOCK() when mmap(2)ing files. > > Previous attempt to unlock amap & anon exposed a race in vnode reference > counting. So be conservative with the code paths that we're not fully > moving > out of the KERNEL_LOCK() to allow us to concentrate on one area at a > time. > ... > > > So here's a first small step. I've been running with this for months > on a few amd64, arm64 and sparc64 boxes without problems; they've been > daily drivers and/or have been building releases and ports. > > Feedback? Objections? OK? > > > Index: sys/kern/syscalls.master > === > RCS file: /cvs/src/sys/kern/syscalls.master,v > retrieving revision 1.221 > diff -u -p -r1.221 syscalls.master > --- sys/kern/syscalls.master 23 Dec 2021 18:50:31 - 1.221 > +++ sys/kern/syscalls.master 31 Dec 2021 09:14:00 - > @@ -126,7 +126,7 @@ > struct sigaction *osa); } > 47STD NOLOCK { gid_t sys_getgid(void); } > 48STD NOLOCK { int sys_sigprocmask(int how, sigset_t mask); } > -49 STD { void *sys_mmap(void *addr, size_t len, int prot, \ > +49 STD NOLOCK { void *sys_mmap(void *addr, size_t len, int prot, \ > int flags, int fd, off_t pos); } > 50STD { int sys_setlogin(const char *namebuf); } > #ifdef ACCOUNTING >
Re: unlock mmap(2) for anonymous mappings
>Now that mpi has unlocked uvm's fault handler, we can unlock the mmap >syscall to handle MAP_ANON without the big lock. ... >So here's a first small step. I've been running with this for months >on a few amd64, arm64 and sparc64 boxes without problems So, 3 architectures have been tested. I read your mail as a request for OKs to commit before the other architectures have been tested. No way. You first find people to help you test the others. Or you ask for the whole community to help ensure the list is complete. But you are begging for the wrong people to respond to you when you include this on the list of questions: > Feedback? Objections? OK? ^^^
Fix GNUism in bsd.dep.mk
Hi, Here at genua, trying to build libpcap sometimes breaks in libpcap with the following error message: | Using $< in a non-suffix rule context is a GNUmake idiom \ |(/data/git/ehrhardt/genuos/os/mk/bsd.dep.mk:47) The bug is in bsd.dep.mk where ${.IMPSRC} (aka $<) is used in a context where it is not neccessarily defined by OpenBSD make. Below is a diff to Use ${.ALLSRC:M*.y} instead of ${.IMPSRC} as the input file for yacc. The issue is with the rule for the grammar.h file that is generated by yacc from grammar.c. You can easily reproduce the bug with the following steps: - build libpcap from scratch: cd .../src/lib/libpcap && make clean all - remove the generated grammar.h file: rm obj*/grammar.h - build libpcap again (incremental build): make In normal builds this does not trigger as grammar.h is implicitly generated by the rule for grammar.c and when make checks for dependencies it simply finds grammar.h uptodate. However, incremental or parallel builds might decide to make grammar.h from grammar.y. Now, why is this only a problem for grammar.h but not for grammar.c? The answer to this question is burried deeply in OpenBSD's mk files. The snippet in bsd.dep.mk that triggers the error is a single rule statement that generates foo.c and foo.h from foo.y with a call to yacc -d. The rule is generated with a loop, i.e. it is not a prefix rule. However, a prefix rule context is required for the use of ${.IMPSRC} aka $<. For the .c file such a prefix rule is provided by bsd.sys.mk and this rule is in scope when make evaluates the yacc rule. However, for .h file generation from a .y file there is no such prefix rule defined in any of the Makefiles. Even if it were the .h suffix is missing from .SUFFIXES and the rule would not be considered. NOTE: The obvious way to fix this would be to use $f instead of ${.IMPSRC}. However, this does not work as $f is then missing the path prefix and yacc won't find it if an obj directory is used. This is probably the reason for the use of ${.IMPSRC} in the first place. regards Christian diff --git a/os/mk/bsd.dep.mk b/os/mk/bsd.dep.mk index 4bd8bf7778..77ae4f 100644 --- a/os/mk/bsd.dep.mk +++ b/os/mk/bsd.dep.mk @@ -44,7 +44,7 @@ ${i:R:S/$/.o/} ${i:R:S/$/.po/} ${i:R:S/$/.so/} ${i:R:S/$/.do/}: $i # loop may not trigger . for f in ${SRCS:M*.y} ${f:.y=.c} ${f:.y=.h}: $f - ${YACC.y} -o ${f:.y=.c} ${.IMPSRC} + ${YACC.y} -o ${f:.y=.c} ${.ALLSRC:M*.y} . endfor CLEANFILES += ${SRCS:M*.y:.y=.h} .endif smime.p7s Description: S/MIME cryptographic signature
less: fix use after free bug
Hi, it is possible to trigger a use after free bug in less with huge files or tight memory constraints. PoC with 100 MB file: dd if=/dev/zero bs=1024 count=102400 | tr '\0' 'a' > less-poc.txt ulimit -d 157286 less less-poc.txt The linebuf and attr buffers in line.c are supposed to never be freed, since they are used for keeping (meta) data of current line in RAM. The linebuf and attr buffers are expanded in expand_linebuf. If linebuf could be expanded but attr could not, then returned pointers are freed, but linebuf variable still points to previous location. Subsequent accesses will therefore trigger a use after free bug. The easiest fix is to keep reallocated linebuf. The size of both buffers differ, but the code still assumes that the previous (smaller) size is the correct one. Thoughts on this approach? Index: line.c === RCS file: /cvs/src/usr.bin/less/line.c,v retrieving revision 1.34 diff -u -p -u -p -r1.34 line.c --- line.c 25 Oct 2021 19:54:29 - 1.34 +++ line.c 31 Dec 2021 13:56:48 - @@ -98,8 +98,10 @@ expand_linebuf(void) char *new_buf = recallocarray(linebuf, size_linebuf, new_size, 1); char *new_attr = recallocarray(attr, size_linebuf, new_size, 1); if (new_buf == NULL || new_attr == NULL) { - free(new_attr); - free(new_buf); + if (new_buf != NULL) + linebuf = new_buf; + if (new_attr != NULL) + attr = new_attr; return (1); } linebuf = new_buf;
unlock mmap(2) for anonymous mappings
Now that mpi has unlocked uvm's fault handler, we can unlock the mmap syscall to handle MAP_ANON without the big lock. sys_mmap() still protects the !MAP_ANON case, i.e. file based mappings, with the KERNEL_LOCK() itself, which is why unlocking the syscall will only change locking behaviour for anonymous mappings. A previous to unlock file based mappings was reverted, see the following from https://marc.info/?l=openbsd-tech&m=160155434212888&w=2 : commit 38802bc07455f2a4f8cdde272850a5eab5dfa6c8 from: mpi date: Wed Oct 7 12:26:20 2020 UTC Do not release the KERNEL_LOCK() when mmap(2)ing files. Previous attempt to unlock amap & anon exposed a race in vnode reference counting. So be conservative with the code paths that we're not fully moving out of the KERNEL_LOCK() to allow us to concentrate on one area at a time. ... So here's a first small step. I've been running with this for months on a few amd64, arm64 and sparc64 boxes without problems; they've been daily drivers and/or have been building releases and ports. Feedback? Objections? OK? Index: sys/kern/syscalls.master === RCS file: /cvs/src/sys/kern/syscalls.master,v retrieving revision 1.221 diff -u -p -r1.221 syscalls.master --- sys/kern/syscalls.master23 Dec 2021 18:50:31 - 1.221 +++ sys/kern/syscalls.master31 Dec 2021 09:14:00 - @@ -126,7 +126,7 @@ struct sigaction *osa); } 47 STD NOLOCK { gid_t sys_getgid(void); } 48 STD NOLOCK { int sys_sigprocmask(int how, sigset_t mask); } -49 STD { void *sys_mmap(void *addr, size_t len, int prot, \ +49 STD NOLOCK { void *sys_mmap(void *addr, size_t len, int prot, \ int flags, int fd, off_t pos); } 50 STD { int sys_setlogin(const char *namebuf); } #ifdef ACCOUNTING