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=160155434212888=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=160155434212888=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
