Re: apldc/aplhidev: enable LEDs in xorg
> Date: Sun, 9 Apr 2023 22:56:01 +0200 > From: Tobias Heider > > This patch enables the capslock LED on apple m1/m2 laptops in xenocara. > Console mode was already working by setting the correct accessop, for > X we are missing an ioctl handler. > > Only tested on apldc but the aplhidev code looks identical so the fix > should be the same. ok kettenis@ > Index: apldc.c > === > RCS file: /cvs/src/sys/arch/arm64/dev/apldc.c,v > retrieving revision 1.6 > diff -u -p -r1.6 apldc.c > --- apldc.c 26 Mar 2023 09:34:06 - 1.6 > +++ apldc.c 9 Apr 2023 20:52:25 - > @@ -1169,6 +1169,9 @@ apldckbd_ioctl(void *v, u_long cmd, cadd > /* XXX: should we set something else? */ > *(u_int *)data = WSKBD_TYPE_USB; > return 0; > + case WSKBDIO_SETLEDS: > + apldckbd_set_leds(v, *(int *)data); > + return 0; > default: > return hidkbd_ioctl(kbd, cmd, data, flag, p); > } > Index: aplhidev.c > === > RCS file: /cvs/src/sys/arch/arm64/dev/aplhidev.c,v > retrieving revision 1.10 > diff -u -p -r1.10 aplhidev.c > --- aplhidev.c21 Nov 2022 14:39:23 - 1.10 > +++ aplhidev.c9 Apr 2023 20:52:25 - > @@ -596,6 +596,9 @@ aplkbd_ioctl(void *v, u_long cmd, caddr_ > /* XXX: should we set something else? */ > *(u_int *)data = WSKBD_TYPE_USB; > return 0; > + case WSKBDIO_SETLEDS: > + aplkbd_set_leds(v, *(int *)data); > + return 0; > default: > return hidkbd_ioctl(kbd, cmd, data, flag, p); > } > >
apldc/aplhidev: enable LEDs in xorg
This patch enables the capslock LED on apple m1/m2 laptops in xenocara. Console mode was already working by setting the correct accessop, for X we are missing an ioctl handler. Only tested on apldc but the aplhidev code looks identical so the fix should be the same. Index: apldc.c === RCS file: /cvs/src/sys/arch/arm64/dev/apldc.c,v retrieving revision 1.6 diff -u -p -r1.6 apldc.c --- apldc.c 26 Mar 2023 09:34:06 - 1.6 +++ apldc.c 9 Apr 2023 20:52:25 - @@ -1169,6 +1169,9 @@ apldckbd_ioctl(void *v, u_long cmd, cadd /* XXX: should we set something else? */ *(u_int *)data = WSKBD_TYPE_USB; return 0; + case WSKBDIO_SETLEDS: + apldckbd_set_leds(v, *(int *)data); + return 0; default: return hidkbd_ioctl(kbd, cmd, data, flag, p); } Index: aplhidev.c === RCS file: /cvs/src/sys/arch/arm64/dev/aplhidev.c,v retrieving revision 1.10 diff -u -p -r1.10 aplhidev.c --- aplhidev.c 21 Nov 2022 14:39:23 - 1.10 +++ aplhidev.c 9 Apr 2023 20:52:25 - @@ -596,6 +596,9 @@ aplkbd_ioctl(void *v, u_long cmd, caddr_ /* XXX: should we set something else? */ *(u_int *)data = WSKBD_TYPE_USB; return 0; + case WSKBDIO_SETLEDS: + aplkbd_set_leds(v, *(int *)data); + return 0; default: return hidkbd_ioctl(kbd, cmd, data, flag, p); }
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 12:17:35PM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > > > I would prefer if every utrace() call is a full line (in other words ulog > > should be line buffered). It makes the regular kdump output more useable. > > Right now you depend on kdump -u to put the lines back together. > > Right. Done line buffering in the new diff below. > > > Whenever I used utrace() I normally passed binary objects to the call so I > > could enrich the ktrace with userland trace info. So if kdump -u is used > > for more then just mallocstats it should have a true binary mode. For > > example gmon.out is a binary format so the above example by semarie@ would > > not work. As usual this can be solved in tree once that problem is hit. > > Right, we can do that when needed. > > This is approaching a state where I'm happy with it. So reviews, tests > appreciated. > > I would like to have the possibility to record a caller deeper in the > stack (as many program use wrappers to call malloc), but > __builtin_return_address only works with a constant argument. So some > magic needed. Will return to that later. > > -Otto the kdump part is ok semarie@. maybe wait a bit a let's others to comment. regarding the malloc part, it reads fine. If I correctly followed the code, ulog() function is only called during malloc_exit() at atexit(3) time. so, the static variables should be fine in multi-threaded context (atexit(3) finalizer is using spinlock to be thread-safe). Thanks. > Index: lib/libc/stdlib/malloc.3 > === > RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v > retrieving revision 1.130 > diff -u -p -r1.130 malloc.3 > --- lib/libc/stdlib/malloc.3 1 Apr 2023 18:47:51 - 1.130 > +++ lib/libc/stdlib/malloc.3 9 Apr 2023 10:16:11 - > @@ -284,10 +284,13 @@ If it has been corrupted, the process is > .It Cm D > .Dq Dump . > .Fn malloc > -will dump statistics to the file > -.Pa ./malloc.out , > -if it already exists, > +will dump statistics using > +.Xr utrace 2 > at exit. > +To record the dump: > +.Dl $ MALLOC_OPTIONS=D ktrace -tu program ... > +To view the dump: > +.Dl $ kdump -u malloc ... > This option requires the library to have been compiled with -DMALLOC_STATS in > order to have any effect. > .It Cm F > Index: lib/libc/stdlib/malloc.c > === > RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v > retrieving revision 1.280 > diff -u -p -r1.280 malloc.c > --- lib/libc/stdlib/malloc.c 5 Apr 2023 06:25:38 - 1.280 > +++ lib/libc/stdlib/malloc.c 9 Apr 2023 10:16:11 - > @@ -1,6 +1,6 @@ > /* $OpenBSD: malloc.c,v 1.280 2023/04/05 06:25:38 otto Exp $ */ > /* > - * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek > + * Copyright (c) 2008, 2010, 2011, 2016, 2023 Otto Moerbeek > * Copyright (c) 2012 Matthew Dempsky > * Copyright (c) 2008 Damien Miller > * Copyright (c) 2000 Poul-Henning Kamp > @@ -23,7 +23,9 @@ > * can buy me a beer in return. Poul-Henning Kamp > */ > > -/* #define MALLOC_STATS */ > +#ifndef SMALL > +#define MALLOC_STATS > +#endif > > #include > #include > @@ -39,8 +41,10 @@ > #include > > #ifdef MALLOC_STATS > +#include > #include > -#include > +#include > +#include > #endif > > #include "thread_private.h" > @@ -225,10 +229,14 @@ struct malloc_readonly { > size_t malloc_guard; /* use guard pages after allocations? */ > #ifdef MALLOC_STATS > int malloc_stats; /* dump statistics at end */ > +#define DO_STATSmopts.malloc_stats > +#else > +#define DO_STATS0 > #endif > u_int32_t malloc_canary;/* Matched against ones in pool */ > }; > > + > /* This object is mapped PROT_READ after initialisation to prevent tampering > */ > static union { > struct malloc_readonly mopts; > @@ -243,15 +251,11 @@ static __dead void wrterror(struct dir_i > __attribute__((__format__ (printf, 2, 3))); > > #ifdef MALLOC_STATS > -void malloc_dump(int, int, struct dir_info *); > +void malloc_dump(void); > PROTO_NORMAL(malloc_dump); > -void malloc_gdump(int); > -PROTO_NORMAL(malloc_gdump); > static void malloc_exit(void); > -#define CALLER __builtin_return_address(0) > -#else > -#define CALLER NULL > #endif > +#define CALLER (DO_STATS ? __builtin_return_address(0) : NULL) > > /* low bits of r->p determine size: 0 means >= page size and r->size holding > * real size, otherwise low bits is the bucket + 1 > @@ -318,9 +322,9 @@ wrterror(struct dir_info *d, char *msg, > dprintf(STDERR_FILENO, "\n"); > > #ifdef MALLOC_STATS > - if (mopts.malloc_stats) > - malloc_gdump(STDERR_FILENO); > -#endif /* MALLOC_STATS */ > + if (DO_STATS) > +
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > Hi, > > > > > > > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > > > > introducing should be two or a single one. > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If > > > > > > run > > > > > > with option D it dumps its state to a malloc.out file at exit. This > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > This is not ideal for pledged processes, as they often have no way > > > > > > to > > > > > > write files. > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > As kdump has no nice way to show those lines without all extras it > > > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > Run : > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > ... > > > > > > $ kdump -hu > > > > > > > > > > > > Feedback appreciated. > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) > > > > is a good > > > > way to get information outside a pledged process. > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > cooperation > > > > from outside to exfiltrate informations (a process with permission to > > > > call > > > > ktrace(2) on this pid). > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > processes would > > > > also get advantage of using it. > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h (no > > > > header). > > > > > > > > Does it would make sens to considere a process using utrace(2) with > > > > several > > > > interleaved records for different sources ? A process with > > > > MALLOC_OPTIONS=D and > > > > profiling enabled for example ? An (another) option on kdump to filter > > > > on utrace > > > > label would be useful in such case, or have -u mandate a label to > > > > filter on. > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > $ kdump -u mallocdumpline > > > > > > > > and for profiling: > > > > > > > > $ kdump -u profil > gmon.out > > > > $ gprof your_program gmon.out > > > > > > > > Thanks. > > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > > part to make it more flexable for different purposes. > > > > Anothew aspect of safety is the information availble in the heap > > itself. I'm pretty sure the addresses of call sites into malloc are > > interesting to attackers. To prevent a program having access to those > > (even if they are stored inside the malloc meta data that is not > > directly accesible to a program), I'm making sure the recording only > > takes place if malloc option D is used. > > > > This is included in a diff with the kdump changes and a few other > > things below. I'm also compiling with MALLOC_STATS if !SMALL. > > > > usage is now: > > > > $ MALLOC_OPTIONS=D ktrace -tu a.out > > $ kdump -u malloc > > > > I would prefer if every utrace() call is a full line (in other words ulog > should be line buffered). It makes the regular kdump output more useable. > Right now you depend on kdump -u to put the lines back together. Right. Done line buffering in the new diff below. > Whenever I used utrace() I normally passed binary objects to the call so I > could enrich the ktrace with userland trace info. So if kdump -u is used > for more then just mallocstats it should have a true binary mode. For > example gmon.out is a binary format so the above example by semarie@ would > not work. As usual this can be solved in tree once that problem is hit. Right, we can do that when needed. This is approaching a state where I'm happy with it. So reviews, tests appreciated. I would like to have the possibility to record a caller deeper in the stack (as many program use wrappers to call malloc), but __builtin_return_address only works with a constant argument. So some magic needed. Will return to that later. -Otto Index: lib/libc/stdlib/malloc.3 === RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v retrieving revision 1.130 diff -u -p -r1.130 malloc.3 --- lib/libc/stdlib/malloc.31 Apr 2023 18:47:51 - 1.130 +++ lib/libc/stdlib/malloc.39 Apr 2023 10:16:11 - @@ -284,10 +284,13 @@ If it has been corrupted, the process is .It Cm D .Dq Dump . .Fn malloc -will dump statistics to the file -.Pa ./malloc.out ,
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > I would prefer if every utrace() call is a full line (in other words ulog > should be line buffered). It makes the regular kdump output more useable. > Right now you depend on kdump -u to put the lines back together. > > Whenever I used utrace() I normally passed binary objects to the call so I > could enrich the ktrace with userland trace info. So if kdump -u is used > for more then just mallocstats it should have a true binary mode. For > example gmon.out is a binary format so the above example by semarie@ would > not work. As usual this can be solved in tree once that problem is hit. Yes, I saw my example wasn't right for gmon.out. but it could be workarounded easily with unvis(1) as the vis(3) options used preserved the reversibility. $ kdump -u profil | unvis > gmon.out I agree it could be done in tree if a binary output in kdump(1) is prefered. Regarding profiling, I wonder if it is currently broken. I am investigating, and will do a bug report once I have enough information. -- Sebastien Marie
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > Hi, > > > > > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > > > introducing should be two or a single one. > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > > > > with option D it dumps its state to a malloc.out file at exit. This > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > This is not ideal for pledged processes, as they often have no way to > > > > > write files. > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > As kdump has no nice way to show those lines without all extras it > > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > Run : > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > ... > > > > > $ kdump -hu > > > > > > > > > > Feedback appreciated. > > > > > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) is > > > a good > > > way to get information outside a pledged process. > > > > > > I tend to think it is safe to use it, as the pledged process need > > > cooperation > > > from outside to exfiltrate informations (a process with permission to > > > call > > > ktrace(2) on this pid). > > > > > > Please note it is a somehow generic problem: at least profiled processes > > > would > > > also get advantage of using it. > > > > > > > > > Regarding kdump options, I think that -u option should implies -h (no > > > header). > > > > > > Does it would make sens to considere a process using utrace(2) with > > > several > > > interleaved records for different sources ? A process with > > > MALLOC_OPTIONS=D and > > > profiling enabled for example ? An (another) option on kdump to filter on > > > utrace > > > label would be useful in such case, or have -u mandate a label to filter > > > on. > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > $ kdump -u mallocdumpline > > > > > > and for profiling: > > > > > > $ kdump -u profil > gmon.out > > > $ gprof your_program gmon.out > > > > > > Thanks. > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > part to make it more flexable for different purposes. > > Anothew aspect of safety is the information availble in the heap > itself. I'm pretty sure the addresses of call sites into malloc are > interesting to attackers. To prevent a program having access to those > (even if they are stored inside the malloc meta data that is not > directly accesible to a program), I'm making sure the recording only > takes place if malloc option D is used. > > This is included in a diff with the kdump changes and a few other > things below. I'm also compiling with MALLOC_STATS if !SMALL. > > usage is now: > > $ MALLOC_OPTIONS=D ktrace -tu a.out > $ kdump -u malloc > I would prefer if every utrace() call is a full line (in other words ulog should be line buffered). It makes the regular kdump output more useable. Right now you depend on kdump -u to put the lines back together. Whenever I used utrace() I normally passed binary objects to the call so I could enrich the ktrace with userland trace info. So if kdump -u is used for more then just mallocstats it should have a true binary mode. For example gmon.out is a binary format so the above example by semarie@ would not work. As usual this can be solved in tree once that problem is hit. > Index: lib/libc/stdlib/malloc.3 > === > RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v > retrieving revision 1.130 > diff -u -p -r1.130 malloc.3 > --- lib/libc/stdlib/malloc.3 1 Apr 2023 18:47:51 - 1.130 > +++ lib/libc/stdlib/malloc.3 9 Apr 2023 07:08:20 - > @@ -284,10 +284,13 @@ If it has been corrupted, the process is > .It Cm D > .Dq Dump . > .Fn malloc > -will dump statistics to the file > -.Pa ./malloc.out , > -if it already exists, > +will dump statistics using > +.Xr utrace 2 > at exit. > +To record the dump: > +.Dl $ MALLOC_OPTIONS=D ktrace -tu program ... > +To view the dump: > +.Dl $ kdump -u malloc ... > This option requires the library to have been compiled with -DMALLOC_STATS in > order to have any effect. > .It Cm F > Index: lib/libc/stdlib/malloc.c > === > RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v > retrieving revision 1.280 > diff -u -p -r1.280 malloc.c > --- lib/libc/stdlib/malloc.c 5 Apr 2023 06:25:38 - 1.280 > +++ lib/libc/stdlib/malloc.c 9 Apr 2023 07:08:20 - > @@ -1,6 +1,6 @@ >
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > part to make it more flexable for different purposes. > > Anothew aspect of safety is the information availble in the heap > itself. I'm pretty sure the addresses of call sites into malloc are > interesting to attackers. To prevent a program having access to those > (even if they are stored inside the malloc meta data that is not > directly accesible to a program), I'm making sure the recording only > takes place if malloc option D is used. > > This is included in a diff with the kdump changes and a few other > things below. I'm also compiling with MALLOC_STATS if !SMALL. > > usage is now: > > $ MALLOC_OPTIONS=D ktrace -tu a.out > $ kdump -u malloc > > -Otto few comments regarding kdump below. I will try to look at malloc part too. > Index: usr.bin/kdump/kdump.1 > === > RCS file: /home/cvs/src/usr.bin/kdump/kdump.1,v > retrieving revision 1.35 > diff -u -p -r1.35 kdump.1 > --- usr.bin/kdump/kdump.1 30 Jul 2022 07:19:30 - 1.35 > +++ usr.bin/kdump/kdump.1 9 Apr 2023 07:08:20 - > @@ -42,6 +42,7 @@ > .Op Fl m Ar maxdata > .Op Fl p Ar pid > .Op Fl t Ar trstr > +.Op Fl u Ar word I would use the term 'label' (instead of 'word'), as it is the way it is called inside utrace(2) man page. > .Sh DESCRIPTION > .Nm > displays the kernel trace files produced with > @@ -106,6 +107,18 @@ See the > option of > .Xr ktrace 1 > for the meaning of the letters. > +.It Fl u Ar word > +Display > +.Xr utrace 2 > +records having > +.XR utrace 2 > +header > +.Ar word > +as strings with > +.Xr vis 3 > +escaping, without > +.Xr ktrace 2 > +header information. > .It Fl X > Display I/O data with hexadecimal data and printable ASCII characters > side by side. > Index: usr.bin/kdump/kdump.c > === > RCS file: /home/cvs/src/usr.bin/kdump/kdump.c,v > retrieving revision 1.156 > diff -u -p -r1.156 kdump.c > --- usr.bin/kdump/kdump.c 17 Feb 2023 18:01:26 - 1.156 > +++ usr.bin/kdump/kdump.c 9 Apr 2023 07:08:20 - > @@ -88,6 +88,7 @@ int needtid, tail, basecol; > char *tracefile = DEF_TRACEFILE; > struct ktr_header ktr_header; > pid_t pid_opt = -1; > +char* utracefilter; > > #define eqs(s1, s2) (strcmp((s1), (s2)) == 0) > > @@ -168,7 +169,7 @@ main(int argc, char *argv[]) > screenwidth = 80; > } > > - while ((ch = getopt(argc, argv, "f:dHlm:np:RTt:xX")) != -1) > + while ((ch = getopt(argc, argv, "f:dHlm:np:RTt:u:xX")) != -1) > switch (ch) { > case 'f': > tracefile = optarg; > @@ -212,6 +213,9 @@ main(int argc, char *argv[]) > if (trpoints < 0) > errx(1, "unknown trace point in %s", optarg); > break; > + case 'u': > + utracefilter = optarg; I think it would make sense to also force trpoints to only user data: trpoints = KTRFAC_USER; this way, if you trace others informations in the dump file, you only get the related utrace(2) entries. -t and -u should be also mutually exclusive. > + break; > case 'x': > iohex = 1; > break; > @@ -246,7 +250,7 @@ main(int argc, char *argv[]) > silent = 0; > if (pid_opt != -1 && pid_opt != ktr_header.ktr_pid) > silent = 1; > - if (silent == 0 && trpoints & (1< + if (utracefilter == NULL && silent == 0 && trpoints & > (1< dumpheader(_header); > ktrlen = ktr_header.ktr_len; > if (ktrlen > size) { > @@ -1254,10 +1258,18 @@ showbufc(int col, unsigned char *dp, siz > static void > showbuf(unsigned char *dp, size_t datalen) > { > - int i, j; > + size_t i, j; > int col = 0, bpl; > unsigned char c; > + char visbuf[5]; > > + if (utracefilter != NULL) { > + for (i = 0; i < datalen; i++) { > + vis(visbuf, dp[i], VIS_SAFE | VIS_OCTAL, 0); > + printf("%s", visbuf); I don't know if it would make a difference or not, but it is possible to use strvis(3) as dp is fixed size (KTR_USER_MAXLEN). so visbuf would be KTR_USER_MAXLEN*4+1 in size. but the way you did is fine too. > + } > + return; > + } > if (iohex == 1) { > putchar('\t'); > col = 8; > @@ -1280,7 +1292,7 @@ showbuf(unsigned char *dp, size_t datale > if (bpl <= 0) > bpl = 1; > for (i = 0; i < datalen; i += bpl) { > -
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > Hi, > > > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > > introducing should be two or a single one. > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > > > with option D it dumps its state to a malloc.out file at exit. This > > > > state can be used to find leaks amongst other things. > > > > > > > > This is not ideal for pledged processes, as they often have no way to > > > > write files. > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > As kdump has no nice way to show those lines without all extras it > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > Run : > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > ... > > > > $ kdump -hu > > > > > > > > Feedback appreciated. > > > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) is a > > good > > way to get information outside a pledged process. > > > > I tend to think it is safe to use it, as the pledged process need > > cooperation > > from outside to exfiltrate informations (a process with permission to call > > ktrace(2) on this pid). > > > > Please note it is a somehow generic problem: at least profiled processes > > would > > also get advantage of using it. > > > > > > Regarding kdump options, I think that -u option should implies -h (no > > header). > > > > Does it would make sens to considere a process using utrace(2) with several > > interleaved records for different sources ? A process with MALLOC_OPTIONS=D > > and > > profiling enabled for example ? An (another) option on kdump to filter on > > utrace > > label would be useful in such case, or have -u mandate a label to filter on. > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > $ kdump -u mallocdumpline > > > > and for profiling: > > > > $ kdump -u profil > gmon.out > > $ gprof your_program gmon.out > > > > Thanks. > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > part to make it more flexable for different purposes. Anothew aspect of safety is the information availble in the heap itself. I'm pretty sure the addresses of call sites into malloc are interesting to attackers. To prevent a program having access to those (even if they are stored inside the malloc meta data that is not directly accesible to a program), I'm making sure the recording only takes place if malloc option D is used. This is included in a diff with the kdump changes and a few other things below. I'm also compiling with MALLOC_STATS if !SMALL. usage is now: $ MALLOC_OPTIONS=D ktrace -tu a.out $ kdump -u malloc -Otto Index: lib/libc/stdlib/malloc.3 === RCS file: /home/cvs/src/lib/libc/stdlib/malloc.3,v retrieving revision 1.130 diff -u -p -r1.130 malloc.3 --- lib/libc/stdlib/malloc.31 Apr 2023 18:47:51 - 1.130 +++ lib/libc/stdlib/malloc.39 Apr 2023 07:08:20 - @@ -284,10 +284,13 @@ If it has been corrupted, the process is .It Cm D .Dq Dump . .Fn malloc -will dump statistics to the file -.Pa ./malloc.out , -if it already exists, +will dump statistics using +.Xr utrace 2 at exit. +To record the dump: +.Dl $ MALLOC_OPTIONS=D ktrace -tu program ... +To view the dump: +.Dl $ kdump -u malloc ... This option requires the library to have been compiled with -DMALLOC_STATS in order to have any effect. .It Cm F Index: lib/libc/stdlib/malloc.c === RCS file: /home/cvs/src/lib/libc/stdlib/malloc.c,v retrieving revision 1.280 diff -u -p -r1.280 malloc.c --- lib/libc/stdlib/malloc.c5 Apr 2023 06:25:38 - 1.280 +++ lib/libc/stdlib/malloc.c9 Apr 2023 07:08:20 - @@ -1,6 +1,6 @@ /* $OpenBSD: malloc.c,v 1.280 2023/04/05 06:25:38 otto Exp $ */ /* - * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek + * Copyright (c) 2008, 2010, 2011, 2016, 2023 Otto Moerbeek * Copyright (c) 2012 Matthew Dempsky * Copyright (c) 2008 Damien Miller * Copyright (c) 2000 Poul-Henning Kamp @@ -23,7 +23,9 @@ * can buy me a beer in return. Poul-Henning Kamp */ -/* #define MALLOC_STATS */ +#ifndef SMALL +#define MALLOC_STATS +#endif #include #include @@ -39,8 +41,10 @@ #include #ifdef MALLOC_STATS +#include #include -#include +#include +#include #endif #include "thread_private.h" @@ -225,10 +229,14 @@ struct malloc_readonly { size_t malloc_guard; /* use guard pages after allocations? */ #ifdef MALLOC_STATS int malloc_stats; /* dump statistics at end */ +#define DO_STATS
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > Hi, > > > > > > This is work in progress. I have to think if the flags to kdump I'm > > > introducing should be two or a single one. > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. If run > > > with option D it dumps its state to a malloc.out file at exit. This > > > state can be used to find leaks amongst other things. > > > > > > This is not ideal for pledged processes, as they often have no way to > > > write files. > > > > > > This changes malloc to use utrace(2) for that. > > > > > > As kdump has no nice way to show those lines without all extras it > > > normally shows, so add two options to it to just show the lines. > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > Run : > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > ... > > > $ kdump -hu > > > > > > Feedback appreciated. > > I can't really comment on malloc(3) stuff, but I agree that utrace(2) is a > good > way to get information outside a pledged process. > > I tend to think it is safe to use it, as the pledged process need cooperation > from outside to exfiltrate informations (a process with permission to call > ktrace(2) on this pid). > > Please note it is a somehow generic problem: at least profiled processes > would > also get advantage of using it. > > > Regarding kdump options, I think that -u option should implies -h (no header). > > Does it would make sens to considere a process using utrace(2) with several > interleaved records for different sources ? A process with MALLOC_OPTIONS=D > and > profiling enabled for example ? An (another) option on kdump to filter on > utrace > label would be useful in such case, or have -u mandate a label to filter on. > > $ MALLOC_OPTIONS=D ktrace -tu your_program > $ kdump -u mallocdumpline > > and for profiling: > > $ kdump -u profil > gmon.out > $ gprof your_program gmon.out > > Thanks. Thanks! Your suggestions make a lot of sense. I'll rework the kdump part to make it more flexable for different purposes. -Otto > -- > Sebastien Marie