Re: limit MSR_INT_PEN_MSG use to < family 16h
On Thu, Jun 10, 2021 at 03:19:43PM +1000, Jonathan Gray wrote: > Ilya Voronin sent a diff to misc to limit MSR_INT_PEN_MSG use to > < AMD family 17h prompted by a problem with an AWS t3a instance. > > https://marc.info/?l=openbsd-misc=162120066715633=2 > > Digging some more the 16h bkdgs have it as RAZ/non-functional as well. > Bits are documented in 15h. > > BKDG for AMD Family 16h Models 00h-0Fh Processors > MSRC001_0055 Interrupt Pending > 63:0 RAZ. > > BKDG for AMD Family 16h Models 30h-3Fh Processors > MSRC001_0055 Interrupt Pending > 63:0 RAZ > > PPR for AMD Family 17h Model 71h B0 > MSRC001_0055 [Reserved.] (Core::X86::Msr::IntPend) > Read-only. Reset: Fixed,___h. > > Change the test to use extended family id while here. > I'd be ok with this if someone reported that it works on a bare metal EPYC, since the fix here is for a virtualized environment (and we don't know what AWS is doing here). -ml > Index: sys/arch/amd64/amd64/lapic.c > === > RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v > retrieving revision 1.57 > diff -u -p -r1.57 lapic.c > --- sys/arch/amd64/amd64/lapic.c 6 Sep 2020 20:50:00 - 1.57 > +++ sys/arch/amd64/amd64/lapic.c 19 May 2021 09:16:37 - > @@ -299,8 +299,7 @@ lapic_set_lvt(void) >*Family 0Fh Processors" >* #32559 revision 3.00 >*/ > - if ((cpu_id & 0x0f00) == 0x0f00 && > - (cpu_id & 0x0fff) >= 0x0004) { > + if (ci->ci_family >= 0xf && ci->ci_family < 0x16) { > uint64_t msr; > > msr = rdmsr(MSR_INT_PEN_MSG); > Index: sys/arch/i386/i386/lapic.c > === > RCS file: /cvs/src/sys/arch/i386/i386/lapic.c,v > retrieving revision 1.47 > diff -u -p -r1.47 lapic.c > --- sys/arch/i386/i386/lapic.c30 Jul 2018 14:19:12 - 1.47 > +++ sys/arch/i386/i386/lapic.c19 May 2021 09:19:41 - > @@ -160,8 +160,7 @@ lapic_set_lvt(void) >*Family 0Fh Processors" >* #32559 revision 3.00 >*/ > - if ((cpu_id & 0x0f00) == 0x0f00 && > - (cpu_id & 0x0fff) >= 0x0004) { > + if (ci->ci_family >= 0xf && ci->ci_family < 0x16) { > uint64_t msr; > > msr = rdmsr(MSR_INT_PEN_MSG); > >
limit MSR_INT_PEN_MSG use to < family 16h
Ilya Voronin sent a diff to misc to limit MSR_INT_PEN_MSG use to < AMD family 17h prompted by a problem with an AWS t3a instance. https://marc.info/?l=openbsd-misc=162120066715633=2 Digging some more the 16h bkdgs have it as RAZ/non-functional as well. Bits are documented in 15h. BKDG for AMD Family 16h Models 00h-0Fh Processors MSRC001_0055 Interrupt Pending 63:0 RAZ. BKDG for AMD Family 16h Models 30h-3Fh Processors MSRC001_0055 Interrupt Pending 63:0 RAZ PPR for AMD Family 17h Model 71h B0 MSRC001_0055 [Reserved.] (Core::X86::Msr::IntPend) Read-only. Reset: Fixed,___h. Change the test to use extended family id while here. Index: sys/arch/amd64/amd64/lapic.c === RCS file: /cvs/src/sys/arch/amd64/amd64/lapic.c,v retrieving revision 1.57 diff -u -p -r1.57 lapic.c --- sys/arch/amd64/amd64/lapic.c6 Sep 2020 20:50:00 - 1.57 +++ sys/arch/amd64/amd64/lapic.c19 May 2021 09:16:37 - @@ -299,8 +299,7 @@ lapic_set_lvt(void) *Family 0Fh Processors" * #32559 revision 3.00 */ - if ((cpu_id & 0x0f00) == 0x0f00 && - (cpu_id & 0x0fff) >= 0x0004) { + if (ci->ci_family >= 0xf && ci->ci_family < 0x16) { uint64_t msr; msr = rdmsr(MSR_INT_PEN_MSG); Index: sys/arch/i386/i386/lapic.c === RCS file: /cvs/src/sys/arch/i386/i386/lapic.c,v retrieving revision 1.47 diff -u -p -r1.47 lapic.c --- sys/arch/i386/i386/lapic.c 30 Jul 2018 14:19:12 - 1.47 +++ sys/arch/i386/i386/lapic.c 19 May 2021 09:19:41 - @@ -160,8 +160,7 @@ lapic_set_lvt(void) *Family 0Fh Processors" * #32559 revision 3.00 */ - if ((cpu_id & 0x0f00) == 0x0f00 && - (cpu_id & 0x0fff) >= 0x0004) { + if (ci->ci_family >= 0xf && ci->ci_family < 0x16) { uint64_t msr; msr = rdmsr(MSR_INT_PEN_MSG);
Re: hidms: don't ignore mice with no x/y coordinates
joshua stein writes: > A bug was reported where a Kensington USB trackball didn't work > properly: > > uhidev4 at uhub0 port 6 configuration 1 interface 0 "Kensington Expert > Wireless TB" rev 2.00/1.02 addr 9 > uhidev4: iclass 3/1, 3 report ids > ums3 at uhidev4 reportid 1 > ums3: mouse has no X report > ums4 at uhidev4 reportid 2: 0 button > wsmouse3 at ums4 mux 0 > > After looking at the HID report descriptor, this device is weird in > that it puts the buttons and wheel on one report and the trackball > X/Y coordinates an another. This causes uhidev to attach two ums > devices, but the first one fails because there are no X/Y reports > found. > > The proper fix is probably to make ums act like umt and use > UHIDEV_CLAIM_MULTIPLE_REPORTID to find all of the necessary reports > and attach to multiple at once if needed. I started working on this > but all of the logic in hidms_setup gets tricky when it has to look > at multiple reports. So an easier fix is to just not consider a > mouse with no X/Y reports invalid. > > Now the device attaches to the first button/wheel report: > > uhidev4 at uhub4 port 4 configuration 1 interface 0 "Kensington Expert > Wireless TB" rev 2.00/1.02 addr 3 > uhidev4: iclass 3/1, 3 report ids > ums1 at uhidev4 reportid 1: 5 buttons, Z and W dir > wsmouse1 at ums1 mux 0 > ums2 at uhidev4 reportid 2: 0 buttons > wsmouse2 at ums2 mux 0 > > Checking dmesglog for 'no X report' yields a lot of results, so this > may help on other devices. OK gnezdo > > > diff --git sys/dev/hid/hidms.c sys/dev/hid/hidms.c > index ab9cd9c797e..92ca89537da 100644 > --- sys/dev/hid/hidms.c > +++ sys/dev/hid/hidms.c > @@ -76,10 +76,9 @@ hidms_setup(struct device *self, struct hidms *ms, > uint32_t quirks, > ms->sc_flags = quirks; > > if (!hid_locate(desc, dlen, HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_X), id, > - hid_input, >sc_loc_x, )) { > - printf("\n%s: mouse has no X report\n", self->dv_xname); > - return ENXIO; > - } > + hid_input, >sc_loc_x, )) > + ms->sc_loc_x.size = 0; > + > switch(flags & MOUSE_FLAGS_MASK) { > case 0: > ms->sc_flags |= HIDMS_ABSX; > @@ -93,10 +92,9 @@ hidms_setup(struct device *self, struct hidms *ms, > uint32_t quirks, > } > > if (!hid_locate(desc, dlen, HID_USAGE2(HUP_GENERIC_DESKTOP, HUG_Y), id, > - hid_input, >sc_loc_y, )) { > - printf("\n%s: mouse has no Y report\n", self->dv_xname); > - return ENXIO; > - } > + hid_input, >sc_loc_y, )) > + ms->sc_loc_y.size = 0; > + > switch(flags & MOUSE_FLAGS_MASK) { > case 0: > ms->sc_flags |= HIDMS_ABSY; > @@ -292,7 +290,7 @@ hidms_attach(struct hidms *ms, const struct > wsmouse_accessops *ops) > #endif > > printf(": %d button%s", > - ms->sc_num_buttons, ms->sc_num_buttons <= 1 ? "" : "s"); > + ms->sc_num_buttons, ms->sc_num_buttons == 1 ? "" : "s"); > switch (ms->sc_flags & (HIDMS_Z | HIDMS_W)) { > case HIDMS_Z: > printf(", Z dir");
Re: add table_procexec in smtpd
Hi, Here is the updated diff, which removes table_proc and adds table_procexec as the default backend when no backend name matches. With this diff, I have the following configuration for smtpd: # $OpenBSD: smtpd.conf,v 1.14 2019/11/26 20:14:38 gilles Exp $ # This is the smtpd server system-wide configuration file. # See smtpd.conf(5) for more information. table aliases aliases:root.t...@bsd.ac listen on socket # To accept external mail, replace with: listen on all # listen on lo0 action "local_mail" mbox alias action "outbound" relay action "bsd.ac" relay host smtp://10.7.0.1 # Uncomment the following to accept external mail for domain "example.org" # # match from any for domain "example.org" action "local_mail" match from local for local action "local_mail" match from local for domain "bsd.ac" action "bsd.ac" match from local for any action "outbound" where my /usr/local/libexec/smtpd/table-aliases contains: #!/bin/ksh user="${1:-r...@bsd.ac}" while read line do reqid="$(echo $line | awk -F'|' '{ print $5; }')" reply="TABLE-RESULT|$reqid|FOUND|$user" echo $reply done < /dev/stdin exit 0 This should hopefully satisfy the requirements for transparency and sanity. I will work on the opensmtpd-extras and make a PR in the github separately, if that sounds fine. Cheers, Aisha diff --git a/usr.sbin/smtpd/parse.y b/usr.sbin/smtpd/parse.y index 011e306ac61..1b0ee5ad38f 100644 --- a/usr.sbin/smtpd/parse.y +++ b/usr.sbin/smtpd/parse.y @@ -2557,13 +2557,6 @@ table: TABLE STRING STRING { config = p+1; } } - if (config != NULL && *config != '/') { - yyerror("invalid backend parameter for table: %s", - $2); - free($2); - free($3); - YYERROR; - } table = table_create(conf, backend, $2, config); if (!table_config(table)) { yyerror("invalid configuration file %s for table %s", diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile index ef8148be8c9..46831d647dc 100644 --- a/usr.sbin/smtpd/smtpctl/Makefile +++ b/usr.sbin/smtpd/smtpctl/Makefile @@ -47,7 +47,7 @@ SRCS+=table.c SRCS+= table_static.c SRCS+= table_db.c SRCS+= table_getpwnam.c -SRCS+= table_proc.c +SRCS+= table_procexec.c SRCS+= unpack_dns.c SRCS+= spfwalk.c diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h index be934112103..221f24fbdc4 100644 --- a/usr.sbin/smtpd/smtpd.h +++ b/usr.sbin/smtpd/smtpd.h @@ -1656,6 +1656,7 @@ int table_regex_match(const char *, const char *); void table_open_all(struct smtpd *); void table_dump_all(struct smtpd *); void table_close_all(struct smtpd *); +const char *table_service_name(enum table_service ); /* to.c */ diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile index b31d4e42224..64e73c3bb70 100644 --- a/usr.sbin/smtpd/smtpd/Makefile +++ b/usr.sbin/smtpd/smtpd/Makefile @@ -62,7 +62,7 @@ SRCS+=compress_gzip.c SRCS+= table_db.c SRCS+= table_getpwnam.c -SRCS+= table_proc.c +SRCS+= table_procexec.c SRCS+= table_static.c SRCS+= queue_fs.c diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c index 1d82d88b81a..a09229ca174 100644 --- a/usr.sbin/smtpd/table.c +++ b/usr.sbin/smtpd/table.c @@ -45,9 +45,8 @@ struct table_backend *table_backend_lookup(const char *); extern struct table_backend table_backend_static; extern struct table_backend table_backend_db; extern struct table_backend table_backend_getpwnam; -extern struct table_backend table_backend_proc; +extern struct table_backend table_backend_procexec; -static const char * table_service_name(enum table_service); static int table_parse_lookup(enum table_service, const char *, const char *, union lookup *); static int parse_sockaddr(struct sockaddr *, int, const char *); @@ -58,7 +57,7 @@ static struct table_backend *backends[] = { _backend_static, _backend_db, _backend_getpwnam, - _backend_proc, + _backend_procexec, NULL }; @@ -77,7 +76,7 @@ table_backend_lookup(const char *backend) return NULL; } -static const char * +const char * table_service_name(enum table_service s) { switch (s) { @@ -208,10 +207,9 @@ table_create(struct smtpd *conf, const char *backend, const char *name, PATH_LIBEXEC"/table-%s\"", backend); } if (stat(path, ) == 0) { - tb = table_backend_lookup("proc"); - (void)strlcpy(path, backend, sizeof(path)); + tb = table_backend_lookup("proc-exec"); if
dt(4): skip probe frames on arm64
Hi, the diff below adds DT_FA_PROFILE and DT_FA_STATIC defines for arm64 to skip the probe context frames. Here is how a typical arm64 stack trace looks with and without diff: dt_pcb_ring_get+0x130 dt_prov_profile_enter+0x90 hardclock+0x1b0 agtimer_intr+0xa4 ampintc_irq_handler+0x1c0 arm_cpu_irq+0x34 handle_el1h_irq+0x70 sched_idle+0x294<-- Diff skips everything above this sched_idle+0x294 proc_trampoline+0x14 ok? Index: dt_dev.c === RCS file: /mount/openbsd/cvs/src/sys/dev/dt/dt_dev.c,v retrieving revision 1.14 diff -u -p -r1.14 dt_dev.c --- dt_dev.c22 May 2021 21:25:38 - 1.14 +++ dt_dev.c9 Jun 2021 20:57:14 - @@ -56,6 +56,9 @@ #if defined(__amd64__) #define DT_FA_PROFILE 5 #define DT_FA_STATIC 2 +#elif defined(__arm64__) +#define DT_FA_PROFILE 7 +#define DT_FA_STATIC 2 #elif defined(__powerpc64__) #define DT_FA_PROFILE 6 #define DT_FA_STATIC 2
Re: add table_procexec in smtpd
> On 9 Jun 2021, at 17:13, Aisha Tammy wrote: > > > > On 6/9/21 10:34 AM, Gilles CHEHADE wrote: >> >>> On 9 Jun 2021, at 15:47, Aisha Tammy wrote: >>> >>> On 6/9/21 5:19 AM, Gilles CHEHADE wrote: Hi, I wrote table_procexec (despite the copyright which I copy-pasted and forgot to replace author) so just providing a bit of insight: >>> Ah, I did not know that. I will fix the header in the next patch >> No problem, here’s a write-up for reference: >> >> https://poolp.org/posts/2020-05-28/may-2020-opensmtpd-6.7.1p1-release-table-procexec-and-many-pocs/ > Yes, this was very helpful and was also where I started from :D >> table_procexec was written as a proof of concept for a new table protocol inspired by the filter protocol to make it easier to write privsep table backends using any language or library available without requiring dependencies in the daemon. I implemented it as a table backend because it was the easiest way to show a working implementation of the protocol without making changes in the daemon, the table_procexec backend sits in between the daemon and “new” tables to translate old protocol queries into new protocol queries and new protocol results into old protocol results, but the intent was to move the underlying implementation of the table API to the protocol and not retain this table proxy. >>> Do you mean that ALL table backends should be replaced by the new model? >>> Yes, it should be possible to do that but I don't know if that should be >>> done simultaneously as introducing procexec. >> Builtin tables within the daemon do not need an update because they are >> built in the lookup process and do not rely on IPC. >> >> Table backends from opensmtpd-extras would all need an update, yes, but the >> change only affects the protocol and not the interface so the change doesn’t >> happen in the backends but in the api library that abstracts communication >> with the daemon, backends just need to be relinked to the new library, since >> the opensmtpd-extras are expected to match specific opensmtpd releases (no >> backwards compatibility) and they are rebuilt whenever a package is created, >> this isn’t as big and hurtful as it looks. >> >> I agree that maybe this should not be done simultaneously to introducing >> procexec but I still don’t think the way it is introduced here is the proper >> way: >> >> Ultimately you want to be able to do: >> >> table foobar mytable:/etc/smtpd/mytable.conf >> >> and not be aware that there’s a table-procexec or a procexec protocol >> ensuring communication with the daemon. >> >> The fact that you introduce “procexec” in a way that requires it to appear >> in the config will make this very hard to hide/remove/change in the future >> as people will start relying on it. > Ah, this makes it a lot more clearer on what should be done. Now I can work > towards this in a better fashion. >> >> Maybe the simplest way is to temporarily introduce procexec as a builtin >> table backend, like you did, but do the necessary work in parse.y and >> table.c to detect that a table backend in smtpd.conf is meant to be executed >> by table-procexec so the daemon can do it transparently. > This might be hard to concurrently with having table_proc present, as it does > exactly this - use table_proc backend if the backend name is not present in > the table names. > So what I could do is to remove table_proc and introduce table_procexec and > simultaneously update the opensmtpd-extras port to use table_procexec API. Yes, table_proc becomes 100% useless once a switch occurs to table_procexec, I see no downside in killing it in favour of table_procexec as long as opensmtpd-extras is updated too so the backends remain available. I have _never_ had any question regarding table_proc, I doubt anyone besides eric@ and I ever wrote custom backends that didn’t end up in -extras so … -extras is probably the only user of table_proc :-) > Does that sound like a feasible approach? > I'll send an updated diff soonish. To me this sounds like the most sensible approach to bring procexec support to smtpd in a way that’s future-proof and that doesn’t break current features. Someone else should comment but this would be how I’d do it Gilles
Re: Match ps pledge name order with pledge(2)
Josh Rickmar writes: > On Wed, Jun 09, 2021 at 06:01:59PM +, Klemens Nanni wrote: >> > There were three promises which are not documented in pledge(2): >> > disklabel, drm, and vmm. I've just left these at the end. >> Sounds good. > > Are the undocumented promises intentional, or bugs in pledge(2)? I'd say simply undocumented. I can help review documentation for the vmm promise, but I'm unfamiliar with the disklabel and drm stuff. > ... > + { PLEDGE_ERROR, "error" }, > + /* undocumented promises */ To my above point, would rather document these in pledge.2 instead of have this comment. This comment isn't helpful. -dv
Re: Match ps pledge name order with pledge(2)
On Wed, Jun 09, 2021 at 06:01:59PM +, Klemens Nanni wrote: > > There were three promises which are not documented in pledge(2): > > disklabel, drm, and vmm. I've just left these at the end. > Sounds good. Are the undocumented promises intentional, or bugs in pledge(2)? > Either way, a small comment explaining `pledgenames[]'s order in > pledge.h might be... in order? Sure. diff 3484b12ed58f55deb62bd2fb604ec61c1292c8c7 /usr/src blob - 6dce461fadda1a98cbe3508a747c0688a0d548ce file + sys/sys/pledge.h --- sys/sys/pledge.h +++ sys/sys/pledge.h @@ -76,42 +76,44 @@ static const struct { uint64_tbits; const char *name; } pledgenames[] = { + /* match pledge(2) order for ps(1) to print */ + { PLEDGE_STDIO, "stdio" }, { PLEDGE_RPATH, "rpath" }, { PLEDGE_WPATH, "wpath" }, { PLEDGE_CPATH, "cpath" }, - { PLEDGE_STDIO, "stdio" }, + { PLEDGE_DPATH, "dpath" }, { PLEDGE_TMPPATH, "tmppath" }, - { PLEDGE_DNS, "dns" }, { PLEDGE_INET, "inet" }, + { PLEDGE_MCAST, "mcast" }, + { PLEDGE_FATTR, "fattr" }, + { PLEDGE_CHOWNUID, "chown" }, { PLEDGE_FLOCK, "flock" }, { PLEDGE_UNIX, "unix" }, - { PLEDGE_ID,"id" }, - { PLEDGE_TAPE, "tape" }, + { PLEDGE_DNS, "dns" }, { PLEDGE_GETPW, "getpw" }, - { PLEDGE_PROC, "proc" }, - { PLEDGE_SETTIME, "settime" }, - { PLEDGE_FATTR, "fattr" }, - { PLEDGE_PROTEXEC, "prot_exec" }, - { PLEDGE_TTY, "tty" }, { PLEDGE_SENDFD,"sendfd" }, { PLEDGE_RECVFD,"recvfd" }, + { PLEDGE_TAPE, "tape" }, + { PLEDGE_TTY, "tty" }, + { PLEDGE_PROC, "proc" }, { PLEDGE_EXEC, "exec" }, - { PLEDGE_ROUTE, "route" }, - { PLEDGE_MCAST, "mcast" }, - { PLEDGE_VMINFO,"vminfo" }, + { PLEDGE_PROTEXEC, "prot_exec" }, + { PLEDGE_SETTIME, "settime" }, { PLEDGE_PS,"ps" }, - { PLEDGE_DISKLABEL, "disklabel" }, + { PLEDGE_VMINFO,"vminfo" }, + { PLEDGE_ID,"id" }, { PLEDGE_PF,"pf" }, + { PLEDGE_ROUTE, "route" }, + { PLEDGE_WROUTE,"wroute" }, { PLEDGE_AUDIO, "audio" }, - { PLEDGE_DPATH, "dpath" }, - { PLEDGE_DRM, "drm" }, - { PLEDGE_VMM, "vmm" }, - { PLEDGE_CHOWNUID, "chown" }, + { PLEDGE_VIDEO, "video" }, { PLEDGE_BPF, "bpf" }, - { PLEDGE_ERROR, "error" }, - { PLEDGE_WROUTE,"wroute" }, { PLEDGE_UNVEIL,"unveil" }, - { PLEDGE_VIDEO, "video" }, + { PLEDGE_ERROR, "error" }, + /* undocumented promises */ + { PLEDGE_DISKLABEL, "disklabel" }, + { PLEDGE_DRM, "drm" }, + { PLEDGE_VMM, "vmm" }, { 0, NULL }, }; #endif
Re: add table_procexec in smtpd
On 6/9/21 10:34 AM, Gilles CHEHADE wrote: On 9 Jun 2021, at 15:47, Aisha Tammy wrote: On 6/9/21 5:19 AM, Gilles CHEHADE wrote: Hi, I wrote table_procexec (despite the copyright which I copy-pasted and forgot to replace author) so just providing a bit of insight: Ah, I did not know that. I will fix the header in the next patch No problem, here’s a write-up for reference: https://poolp.org/posts/2020-05-28/may-2020-opensmtpd-6.7.1p1-release-table-procexec-and-many-pocs/ Yes, this was very helpful and was also where I started from :D table_procexec was written as a proof of concept for a new table protocol inspired by the filter protocol to make it easier to write privsep table backends using any language or library available without requiring dependencies in the daemon. I implemented it as a table backend because it was the easiest way to show a working implementation of the protocol without making changes in the daemon, the table_procexec backend sits in between the daemon and “new” tables to translate old protocol queries into new protocol queries and new protocol results into old protocol results, but the intent was to move the underlying implementation of the table API to the protocol and not retain this table proxy. Do you mean that ALL table backends should be replaced by the new model? Yes, it should be possible to do that but I don't know if that should be done simultaneously as introducing procexec. Builtin tables within the daemon do not need an update because they are built in the lookup process and do not rely on IPC. Table backends from opensmtpd-extras would all need an update, yes, but the change only affects the protocol and not the interface so the change doesn’t happen in the backends but in the api library that abstracts communication with the daemon, backends just need to be relinked to the new library, since the opensmtpd-extras are expected to match specific opensmtpd releases (no backwards compatibility) and they are rebuilt whenever a package is created, this isn’t as big and hurtful as it looks. I agree that maybe this should not be done simultaneously to introducing procexec but I still don’t think the way it is introduced here is the proper way: Ultimately you want to be able to do: table foobar mytable:/etc/smtpd/mytable.conf and not be aware that there’s a table-procexec or a procexec protocol ensuring communication with the daemon. The fact that you introduce “procexec” in a way that requires it to appear in the config will make this very hard to hide/remove/change in the future as people will start relying on it. Ah, this makes it a lot more clearer on what should be done. Now I can work towards this in a better fashion. Maybe the simplest way is to temporarily introduce procexec as a builtin table backend, like you did, but do the necessary work in parse.y and table.c to detect that a table backend in smtpd.conf is meant to be executed by table-procexec so the daemon can do it transparently. This might be hard to concurrently with having table_proc present, as it does exactly this - use table_proc backend if the backend name is not present in the table names. So what I could do is to remove table_proc and introduce table_procexec and simultaneously update the opensmtpd-extras port to use table_procexec API. Does that sound like a feasible approach? I'll send an updated diff soonish. Cheers, Aisha This way, if or when the table API relies on the procexec protocol instead of the old protocol, the table-procexec layer can be removed and people will not see a difference. Not my call but I don’t think what you’re proposing is the proper way to integrate it. There are a few downsides in plugging it this way but the most annoying one is that instead of starting a table with a configuration parameter, this is tricking it into executing the table_procexec and using the configuration path as the path to the “new” table, which means that you have no way to provide the new table with a configuration for itself. I don't know if this is true. I was able to provide parameters to the procexec executable via Yes, it works (and is how I did my testing), but this is kind of abusing the mechanism it is built upon. In theory you should provide the backend + a configuration file path: table aliases aliases_procexec:/path/to/config not: table aliases "proc-exec:/usr/local/bin/aliases_procexec root.t...@bsd.ac" What you did works but is not how it is intended to work and will be at odds with other backends that expect a config file or a table mapping. I think your diff is very close to something nice but it lacks some adaptations in parse.y / table.c to hide the plumbing of table_procexec which should remain invisible to the user IMO. This is just my 2cts, maybe others have a different opinion on this of course Gilles
Re: add table_procexec in smtpd
> On 9 Jun 2021, at 15:47, Aisha Tammy wrote: > > On 6/9/21 5:19 AM, Gilles CHEHADE wrote: >> Hi, >> >> I wrote table_procexec (despite the copyright which I copy-pasted and forgot >> to replace author) so just providing a bit of insight: > Ah, I did not know that. I will fix the header in the next patch No problem, here’s a write-up for reference: https://poolp.org/posts/2020-05-28/may-2020-opensmtpd-6.7.1p1-release-table-procexec-and-many-pocs/ >> table_procexec was written as a proof of concept for a new table protocol >> inspired by the filter protocol to make it easier to write privsep table >> backends using any language or library available without requiring >> dependencies in the daemon. >> >> I implemented it as a table backend because it was the easiest way to show a >> working implementation of the protocol without making changes in the daemon, >> the table_procexec backend sits in between the daemon and “new” tables to >> translate old protocol queries into new protocol queries and new protocol >> results into old protocol results, >> but the intent was to move the underlying implementation of the table API to >> the protocol and not retain this table proxy. > Do you mean that ALL table backends should be replaced by the new model? Yes, > it should be possible to do that but I don't know if that should be done > simultaneously as introducing procexec. Builtin tables within the daemon do not need an update because they are built in the lookup process and do not rely on IPC. Table backends from opensmtpd-extras would all need an update, yes, but the change only affects the protocol and not the interface so the change doesn’t happen in the backends but in the api library that abstracts communication with the daemon, backends just need to be relinked to the new library, since the opensmtpd-extras are expected to match specific opensmtpd releases (no backwards compatibility) and they are rebuilt whenever a package is created, this isn’t as big and hurtful as it looks. I agree that maybe this should not be done simultaneously to introducing procexec but I still don’t think the way it is introduced here is the proper way: Ultimately you want to be able to do: table foobar mytable:/etc/smtpd/mytable.conf and not be aware that there’s a table-procexec or a procexec protocol ensuring communication with the daemon. The fact that you introduce “procexec” in a way that requires it to appear in the config will make this very hard to hide/remove/change in the future as people will start relying on it. Maybe the simplest way is to temporarily introduce procexec as a builtin table backend, like you did, but do the necessary work in parse.y and table.c to detect that a table backend in smtpd.conf is meant to be executed by table-procexec so the daemon can do it transparently. This way, if or when the table API relies on the procexec protocol instead of the old protocol, the table-procexec layer can be removed and people will not see a difference. >> Not my call but I don’t think what you’re proposing is the proper way to >> integrate it. >> >> There are a few downsides in plugging it this way but the most annoying one >> is that instead of starting a table with a configuration parameter, this is >> tricking it into executing the table_procexec and using the configuration >> path as the path to the “new” table, which means that you have no way to >> provide the new table with a configuration for itself. > I don't know if this is true. I was able to provide parameters to the > procexec executable via > Yes, it works (and is how I did my testing), but this is kind of abusing the mechanism it is built upon. In theory you should provide the backend + a configuration file path: table aliases aliases_procexec:/path/to/config not: table aliases "proc-exec:/usr/local/bin/aliases_procexec root.t...@bsd.ac" What you did works but is not how it is intended to work and will be at odds with other backends that expect a config file or a table mapping. I think your diff is very close to something nice but it lacks some adaptations in parse.y / table.c to hide the plumbing of table_procexec which should remain invisible to the user IMO. This is just my 2cts, maybe others have a different opinion on this of course Gilles
Re: Match ps pledge name order with pledge(2)
On Wed, Jun 09, 2021 at 10:42:06AM -0400, Josh Rickmar wrote: > I was surprised to find that ps -O pledge did not list the pledge > promise names in the same order as the pledge(2) manpage. Besides > lacking consistency, this was also making it difficult to quickly find > which promises are not granted to a process which requires most of > them (e.g. chrome). I was working with pledge the last few days and kept looking at "ps -o command,pledge" in a tight loop to watch the prorams's promises shrink over time. The same thought occured to me: it would only feel natural if ps(1) printed them in the same order pledge(2) mentions them, simply because that is the development model I had been following (reading the list and constructing the promise string in source code). > I figure that the manpage is probably the more consulted reference, > and the order that is preferred, so the patch below reorders the > promise names in pledge.h to match. I'd argue it is the only order developers consult, that's what manpages are for. > There were three promises which are not documented in pledge(2): > disklabel, drm, and vmm. I've just left these at the end. Sounds good. Either way, a small comment explaining `pledgenames[]'s order in pledge.h might be... in order? > diff 3484b12ed58f55deb62bd2fb604ec61c1292c8c7 /usr/src > blob - 6dce461fadda1a98cbe3508a747c0688a0d548ce > file + sys/sys/pledge.h > --- sys/sys/pledge.h > +++ sys/sys/pledge.h > @@ -76,42 +76,42 @@ static const struct { > uint64_tbits; > const char *name; /* match pledge(2) order for ps(1) to print */ > } pledgenames[] = { > + { PLEDGE_STDIO, "stdio" }, > { PLEDGE_RPATH, "rpath" }, > { PLEDGE_WPATH, "wpath" }, > { PLEDGE_CPATH, "cpath" }, > - { PLEDGE_STDIO, "stdio" }, > + { PLEDGE_DPATH, "dpath" }, > { PLEDGE_TMPPATH, "tmppath" }, > - { PLEDGE_DNS, "dns" }, > { PLEDGE_INET, "inet" }, > + { PLEDGE_MCAST, "mcast" }, > + { PLEDGE_FATTR, "fattr" }, > + { PLEDGE_CHOWNUID, "chown" }, > { PLEDGE_FLOCK, "flock" }, > { PLEDGE_UNIX, "unix" }, > - { PLEDGE_ID,"id" }, > - { PLEDGE_TAPE, "tape" }, > + { PLEDGE_DNS, "dns" }, > { PLEDGE_GETPW, "getpw" }, > - { PLEDGE_PROC, "proc" }, > - { PLEDGE_SETTIME, "settime" }, > - { PLEDGE_FATTR, "fattr" }, > - { PLEDGE_PROTEXEC, "prot_exec" }, > - { PLEDGE_TTY, "tty" }, > { PLEDGE_SENDFD,"sendfd" }, > { PLEDGE_RECVFD,"recvfd" }, > + { PLEDGE_TAPE, "tape" }, > + { PLEDGE_TTY, "tty" }, > + { PLEDGE_PROC, "proc" }, > { PLEDGE_EXEC, "exec" }, > - { PLEDGE_ROUTE, "route" }, > - { PLEDGE_MCAST, "mcast" }, > - { PLEDGE_VMINFO,"vminfo" }, > + { PLEDGE_PROTEXEC, "prot_exec" }, > + { PLEDGE_SETTIME, "settime" }, > { PLEDGE_PS,"ps" }, > - { PLEDGE_DISKLABEL, "disklabel" }, > + { PLEDGE_VMINFO,"vminfo" }, > + { PLEDGE_ID,"id" }, > { PLEDGE_PF,"pf" }, > + { PLEDGE_ROUTE, "route" }, > + { PLEDGE_WROUTE,"wroute" }, > { PLEDGE_AUDIO, "audio" }, > - { PLEDGE_DPATH, "dpath" }, > - { PLEDGE_DRM, "drm" }, > - { PLEDGE_VMM, "vmm" }, > - { PLEDGE_CHOWNUID, "chown" }, > + { PLEDGE_VIDEO, "video" }, > { PLEDGE_BPF, "bpf" }, > - { PLEDGE_ERROR, "error" }, > - { PLEDGE_WROUTE,"wroute" }, > { PLEDGE_UNVEIL,"unveil" }, > - { PLEDGE_VIDEO, "video" }, > + { PLEDGE_ERROR, "error" }, /* undocumented promises */ > + { PLEDGE_DISKLABEL, "disklabel" }, > + { PLEDGE_DRM, "drm" }, > + { PLEDGE_VMM, "vmm" }, > { 0, NULL }, > }; > #endif >
Re: add table_procexec in smtpd
On 6/9/21 5:19 AM, Gilles CHEHADE wrote: Hi, I wrote table_procexec (despite the copyright which I copy-pasted and forgot to replace author) so just providing a bit of insight: Ah, I did not know that. I will fix the header in the next patch table_procexec was written as a proof of concept for a new table protocol inspired by the filter protocol to make it easier to write privsep table backends using any language or library available without requiring dependencies in the daemon. I implemented it as a table backend because it was the easiest way to show a working implementation of the protocol without making changes in the daemon, the table_procexec backend sits in between the daemon and “new” tables to translate old protocol queries into new protocol queries and new protocol results into old protocol results, but the intent was to move the underlying implementation of the table API to the protocol and not retain this table proxy. Do you mean that ALL table backends should be replaced by the new model? Yes, it should be possible to do that but I don't know if that should be done simultaneously as introducing procexec. Not my call but I don’t think what you’re proposing is the proper way to integrate it. There are a few downsides in plugging it this way but the most annoying one is that instead of starting a table with a configuration parameter, this is tricking it into executing the table_procexec and using the configuration path as the path to the “new” table, which means that you have no way to provide the new table with a configuration for itself. I don't know if this is true. I was able to provide parameters to the procexec executable via $ cat /etc/mail/smtpd.conf # $OpenBSD: smtpd.conf,v 1.14 2019/11/26 20:14:38 gilles Exp $ # This is the smtpd server system-wide configuration file. # See smtpd.conf(5) for more information. table paliases file:/etc/mail/aliases table aliases "proc-exec:/usr/local/bin/aliases_procexec root.t...@bsd.ac" listen on socket # To accept external mail, replace with: listen on all # listen on lo0 action "local_mail" mbox alias action "outbound" relay action "bsd.ac" relay host smtp://10.7.0.1 # Uncomment the following to accept external mail for domain "example.org" # # match from any for domain "example.org" action "local_mail" match from local for local action "local_mail" match from local for domain "bsd.ac" action "bsd.ac" match from local for any action "outbound" $ cat /usr/local/bin/aliases_procexec #!/bin/ksh user="${1:-r...@bsd.ac}" while read line do reqid="$(echo $line | awk -F'|' '{ print $5; }')" reply="TABLE-RESULT|$reqid|FOUND|$user" echo $reply done < /dev/stdin exit 0 Does this not work as providing configuration parameters? So if we replace all backends by procexec model it will be possible to have configuration such as table aliasesA file /etc/mail/aliases table aliasesB db /etc/mail/aliases.db table aliasesC /usr/local/bin/aliases_procexec "root.t...@bsd.ac" table where the 'file', 'db' can be executables in /usr/libexec/smtpd or absolute paths. This may be a possible thing to do but maybe it can be done after procexec is tested a bit. Hopefully I've addressed the proper concerns. Best, Aisha Gilles On 8 Jun 2021, at 23:04, Aisha Tammy wrote: Hi, I've attached a slightly updated patch for the procexec. Ping for someone to take a look :) Cheers, Aisha diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile index ef8148be8c9..2e8beff1ad1 100644 --- a/usr.sbin/smtpd/smtpctl/Makefile +++ b/usr.sbin/smtpd/smtpctl/Makefile @@ -48,6 +48,7 @@ SRCS+=table_static.c SRCS+= table_db.c SRCS+= table_getpwnam.c SRCS+= table_proc.c +SRCS+= table_procexec.c SRCS+= unpack_dns.c SRCS+= spfwalk.c diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h index be934112103..221f24fbdc4 100644 --- a/usr.sbin/smtpd/smtpd.h +++ b/usr.sbin/smtpd/smtpd.h @@ -1656,6 +1656,7 @@ int table_regex_match(const char *, const char *); voidtable_open_all(struct smtpd *); voidtable_dump_all(struct smtpd *); voidtable_close_all(struct smtpd *); +const char *table_service_name(enum table_service ); /* to.c */ diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile index b31d4e42224..debfc8d8ab7 100644 --- a/usr.sbin/smtpd/smtpd/Makefile +++ b/usr.sbin/smtpd/smtpd/Makefile @@ -63,6 +63,7 @@ SRCS+=compress_gzip.c SRCS+= table_db.c SRCS+= table_getpwnam.c SRCS+= table_proc.c +SRCS+= table_procexec.c SRCS+= table_static.c SRCS+= queue_fs.c diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c index 1d82d88b81a..0c67d205065 100644 --- a/usr.sbin/smtpd/table.c +++ b/usr.sbin/smtpd/table.c @@ -46,8 +46,8 @@ extern struct table_backend table_backend_static; extern struct
Re: Match ps pledge name order with pledge(2)
On Wed, Jun 09, 2021 at 09:01:34AM -0600, Theo de Raadt wrote: > Josh Rickmar wrote: > > > I figure that the manpage is probably the more consulted reference, > > and the order that is preferred, so the patch below reorders the > > promise names in pledge.h to match. > > The current array was value-sorted (by the bit value) to allow binary > search. However no code is actually using binary search. Honestly it > will be hard to maintain this correctly in the future because of the > symbolic names overlaying the bit values. > > The order of the manual pages has come up in discussion before. Some folk > wanted them to be in alphabetic order, but I pushed back, because the order > we use is better for learning incrementally. > > So we have 3 orders to consider: bit order, name order, or man page order. > > My gut reaction is to agree -- man page order is the way to go. > > Let's wait a little while and see what others say. it is fine with me. thanks. -- Sebastien Marie
Re: Match ps pledge name order with pledge(2)
Josh Rickmar wrote: > I figure that the manpage is probably the more consulted reference, > and the order that is preferred, so the patch below reorders the > promise names in pledge.h to match. The current array was value-sorted (by the bit value) to allow binary search. However no code is actually using binary search. Honestly it will be hard to maintain this correctly in the future because of the symbolic names overlaying the bit values. The order of the manual pages has come up in discussion before. Some folk wanted them to be in alphabetic order, but I pushed back, because the order we use is better for learning incrementally. So we have 3 orders to consider: bit order, name order, or man page order. My gut reaction is to agree -- man page order is the way to go. Let's wait a little while and see what others say. side-comment: there is this weird "chown" vs "chownuid" split, which this mapping table doesn't understand or expose properly, as it cannot be mapped backwards. I promised myself I would get back to fixing that years ago and it seems I dropped it.. > There were three promises which are not documented in pledge(2): > disklabel, drm, and vmm. I've just left these at the end. > > diff 3484b12ed58f55deb62bd2fb604ec61c1292c8c7 /usr/src > blob - 6dce461fadda1a98cbe3508a747c0688a0d548ce > file + sys/sys/pledge.h > --- sys/sys/pledge.h > +++ sys/sys/pledge.h > @@ -76,42 +76,42 @@ static const struct { > uint64_tbits; > const char *name; > } pledgenames[] = { > + { PLEDGE_STDIO, "stdio" }, > { PLEDGE_RPATH, "rpath" }, > { PLEDGE_WPATH, "wpath" }, > { PLEDGE_CPATH, "cpath" }, > - { PLEDGE_STDIO, "stdio" }, > + { PLEDGE_DPATH, "dpath" }, > { PLEDGE_TMPPATH, "tmppath" }, > - { PLEDGE_DNS, "dns" }, > { PLEDGE_INET, "inet" }, > + { PLEDGE_MCAST, "mcast" }, > + { PLEDGE_FATTR, "fattr" }, > + { PLEDGE_CHOWNUID, "chown" }, > { PLEDGE_FLOCK, "flock" }, > { PLEDGE_UNIX, "unix" }, > - { PLEDGE_ID,"id" }, > - { PLEDGE_TAPE, "tape" }, > + { PLEDGE_DNS, "dns" }, > { PLEDGE_GETPW, "getpw" }, > - { PLEDGE_PROC, "proc" }, > - { PLEDGE_SETTIME, "settime" }, > - { PLEDGE_FATTR, "fattr" }, > - { PLEDGE_PROTEXEC, "prot_exec" }, > - { PLEDGE_TTY, "tty" }, > { PLEDGE_SENDFD,"sendfd" }, > { PLEDGE_RECVFD,"recvfd" }, > + { PLEDGE_TAPE, "tape" }, > + { PLEDGE_TTY, "tty" }, > + { PLEDGE_PROC, "proc" }, > { PLEDGE_EXEC, "exec" }, > - { PLEDGE_ROUTE, "route" }, > - { PLEDGE_MCAST, "mcast" }, > - { PLEDGE_VMINFO,"vminfo" }, > + { PLEDGE_PROTEXEC, "prot_exec" }, > + { PLEDGE_SETTIME, "settime" }, > { PLEDGE_PS,"ps" }, > - { PLEDGE_DISKLABEL, "disklabel" }, > + { PLEDGE_VMINFO,"vminfo" }, > + { PLEDGE_ID,"id" }, > { PLEDGE_PF,"pf" }, > + { PLEDGE_ROUTE, "route" }, > + { PLEDGE_WROUTE,"wroute" }, > { PLEDGE_AUDIO, "audio" }, > - { PLEDGE_DPATH, "dpath" }, > - { PLEDGE_DRM, "drm" }, > - { PLEDGE_VMM, "vmm" }, > - { PLEDGE_CHOWNUID, "chown" }, > + { PLEDGE_VIDEO, "video" }, > { PLEDGE_BPF, "bpf" }, > - { PLEDGE_ERROR, "error" }, > - { PLEDGE_WROUTE,"wroute" }, > { PLEDGE_UNVEIL,"unveil" }, > - { PLEDGE_VIDEO, "video" }, > + { PLEDGE_ERROR, "error" }, > + { PLEDGE_DISKLABEL, "disklabel" }, > + { PLEDGE_DRM, "drm" }, > + { PLEDGE_VMM, "vmm" }, > { 0, NULL }, > }; > #endif >
Match ps pledge name order with pledge(2)
I was surprised to find that ps -O pledge did not list the pledge promise names in the same order as the pledge(2) manpage. Besides lacking consistency, this was also making it difficult to quickly find which promises are not granted to a process which requires most of them (e.g. chrome). I figure that the manpage is probably the more consulted reference, and the order that is preferred, so the patch below reorders the promise names in pledge.h to match. There were three promises which are not documented in pledge(2): disklabel, drm, and vmm. I've just left these at the end. diff 3484b12ed58f55deb62bd2fb604ec61c1292c8c7 /usr/src blob - 6dce461fadda1a98cbe3508a747c0688a0d548ce file + sys/sys/pledge.h --- sys/sys/pledge.h +++ sys/sys/pledge.h @@ -76,42 +76,42 @@ static const struct { uint64_tbits; const char *name; } pledgenames[] = { + { PLEDGE_STDIO, "stdio" }, { PLEDGE_RPATH, "rpath" }, { PLEDGE_WPATH, "wpath" }, { PLEDGE_CPATH, "cpath" }, - { PLEDGE_STDIO, "stdio" }, + { PLEDGE_DPATH, "dpath" }, { PLEDGE_TMPPATH, "tmppath" }, - { PLEDGE_DNS, "dns" }, { PLEDGE_INET, "inet" }, + { PLEDGE_MCAST, "mcast" }, + { PLEDGE_FATTR, "fattr" }, + { PLEDGE_CHOWNUID, "chown" }, { PLEDGE_FLOCK, "flock" }, { PLEDGE_UNIX, "unix" }, - { PLEDGE_ID,"id" }, - { PLEDGE_TAPE, "tape" }, + { PLEDGE_DNS, "dns" }, { PLEDGE_GETPW, "getpw" }, - { PLEDGE_PROC, "proc" }, - { PLEDGE_SETTIME, "settime" }, - { PLEDGE_FATTR, "fattr" }, - { PLEDGE_PROTEXEC, "prot_exec" }, - { PLEDGE_TTY, "tty" }, { PLEDGE_SENDFD,"sendfd" }, { PLEDGE_RECVFD,"recvfd" }, + { PLEDGE_TAPE, "tape" }, + { PLEDGE_TTY, "tty" }, + { PLEDGE_PROC, "proc" }, { PLEDGE_EXEC, "exec" }, - { PLEDGE_ROUTE, "route" }, - { PLEDGE_MCAST, "mcast" }, - { PLEDGE_VMINFO,"vminfo" }, + { PLEDGE_PROTEXEC, "prot_exec" }, + { PLEDGE_SETTIME, "settime" }, { PLEDGE_PS,"ps" }, - { PLEDGE_DISKLABEL, "disklabel" }, + { PLEDGE_VMINFO,"vminfo" }, + { PLEDGE_ID,"id" }, { PLEDGE_PF,"pf" }, + { PLEDGE_ROUTE, "route" }, + { PLEDGE_WROUTE,"wroute" }, { PLEDGE_AUDIO, "audio" }, - { PLEDGE_DPATH, "dpath" }, - { PLEDGE_DRM, "drm" }, - { PLEDGE_VMM, "vmm" }, - { PLEDGE_CHOWNUID, "chown" }, + { PLEDGE_VIDEO, "video" }, { PLEDGE_BPF, "bpf" }, - { PLEDGE_ERROR, "error" }, - { PLEDGE_WROUTE,"wroute" }, { PLEDGE_UNVEIL,"unveil" }, - { PLEDGE_VIDEO, "video" }, + { PLEDGE_ERROR, "error" }, + { PLEDGE_DISKLABEL, "disklabel" }, + { PLEDGE_DRM, "drm" }, + { PLEDGE_VMM, "vmm" }, { 0, NULL }, }; #endif
[PATCH] Libc: standardize string functions to match iso-c99
Here is the diff: I apologize for not being able to send the file itself diff --git a/games/arithmetic/arithmetic.c b/games/arithmetic/arithmetic.c index 3b872ae1b03..d54894f9dbc 100644 --- a/games/arithmetic/arithmetic.c +++ b/games/arithmetic/arithmetic.c @@ -130,7 +130,7 @@ main(int argc, char *argv[]) /* Now ask the questions. */ for (;;) { - for (cnt = NQUESTS; cnt--;) + for (cnt = NQUESTS; cnt; cnt--) if (problem() == EOF) intr(0); /* Print score and exit */ showstats(); diff --git a/lib/libc/string/memccpy.3 b/lib/libc/string/memccpy.3 index 98326d68713..f471702ef01 100644 --- a/lib/libc/string/memccpy.3 +++ b/lib/libc/string/memccpy.3 @@ -38,7 +38,7 @@ .Sh SYNOPSIS .In string.h .Ft void * -.Fn memccpy "void *dst" "const void *src" "int c" "size_t len" +.Fn memccpy "void * restrict dst" "const void * restrict src" "int c" "size_t len" .Sh DESCRIPTION The .Fn memccpy diff --git a/lib/libc/string/memccpy.c b/lib/libc/string/memccpy.c index 635061b8cb9..ce5fbd38dc6 100644 --- a/lib/libc/string/memccpy.c +++ b/lib/libc/string/memccpy.c @@ -32,18 +32,18 @@ #include void * -memccpy(void *t, const void *f, int c, size_t n) +memccpy(void * __restrict t, const void * __restrict f, int c, size_t n) { if (n) { unsigned char *tp = t; const unsigned char *fp = f; - unsigned char uc = c; + const unsigned char uc = (unsigned char)c; do { if ((*tp++ = *fp++) == uc) return (tp); } while (--n != 0); } - return (0); + return (NULL); } DEF_WEAK(memccpy); diff --git a/lib/libc/string/memchr.c b/lib/libc/string/memchr.c index a6a4bd60d03..4ec8f4b36dd 100644 --- a/lib/libc/string/memchr.c +++ b/lib/libc/string/memchr.c @@ -38,10 +38,12 @@ memchr(const void *s, int c, size_t n) { if (n != 0) { const unsigned char *p = s; + const unsigned char uc = (unsigned char)c; do { - if (*p++ == (unsigned char)c) - return ((void *)(p - 1)); + if (*p == uc) + return ((void *)p); + p++; } while (--n != 0); } return (NULL); diff --git a/lib/libc/string/memcmp.c b/lib/libc/string/memcmp.c index 0df2c54d2af..32c332a6ec2 100644 --- a/lib/libc/string/memcmp.c +++ b/lib/libc/string/memcmp.c @@ -43,8 +43,9 @@ memcmp(const void *s1, const void *s2, size_t n) const unsigned char *p1 = s1, *p2 = s2; do { - if (*p1++ != *p2++) - return (*--p1 - *--p2); + if (*p1 != *p2) + return (*p1 - *p2); + p1++, p2++; } while (--n != 0); } return (0); diff --git a/lib/libc/string/memcpy.3 b/lib/libc/string/memcpy.3 index 8df2a785b99..92bbdb5829e 100644 --- a/lib/libc/string/memcpy.3 +++ b/lib/libc/string/memcpy.3 @@ -40,7 +40,7 @@ .Sh SYNOPSIS .In string.h .Ft void * -.Fn memcpy "void *dst" "const void *src" "size_t len" +.Fn memcpy "void * restrict dst" "const void * restrict src" "size_t len" .Sh DESCRIPTION The .Fn memcpy @@ -69,7 +69,7 @@ function returns the original value of The .Fn memcpy function conforms to -.St -ansiC . +.St -isoC-99. .Sh HISTORY The .Fn memcpy diff --git a/lib/libc/string/memcpy.c b/lib/libc/string/memcpy.c index 19fddc0ab5f..6f4fc748c39 100644 --- a/lib/libc/string/memcpy.c +++ b/lib/libc/string/memcpy.c @@ -39,7 +39,7 @@ * sizeof(word) MUST BE A POWER OF TWO * SO THAT wmask BELOW IS ALL ONES */ -typedeflong word; /* "word" used for optimal copy speed */ +typedefunsigned long word; /* "word" used for optimal copy speed */ #definewsize sizeof(word) #definewmask (wsize - 1) @@ -50,10 +50,10 @@ static const char backwards_msg[] = ": backwards memcpy"; * Copy a block of memory, not handling overlap. */ void * -memcpy(void *dst0, const void *src0, size_t length) +memcpy(void * __restrict dst0, const void * __restrict src0, size_t length) { - char *dst = dst0; - const char *src = src0; + char *dst = (char *)dst0; + const char *src = (const char *)src0; size_t t; if (length == 0 || dst == src) /* nothing to do */ @@ -83,13 +83,13 @@ memcpy(void *dst0, const void *src0, size_t length) /* * Copy forward. */ - t = (long)src; /* only need low bits */ - if ((t | (long)dst) & wmask) { + t = (word)src; /* only need low bits */ + if ((t | (word)dst) & wmask) { /* * Try to align operands. This cannot be done
Re: add table_procexec in smtpd
Hi, I wrote table_procexec (despite the copyright which I copy-pasted and forgot to replace author) so just providing a bit of insight: table_procexec was written as a proof of concept for a new table protocol inspired by the filter protocol to make it easier to write privsep table backends using any language or library available without requiring dependencies in the daemon. I implemented it as a table backend because it was the easiest way to show a working implementation of the protocol without making changes in the daemon, the table_procexec backend sits in between the daemon and “new” tables to translate old protocol queries into new protocol queries and new protocol results into old protocol results, but the intent was to move the underlying implementation of the table API to the protocol and not retain this table proxy. Not my call but I don’t think what you’re proposing is the proper way to integrate it. There are a few downsides in plugging it this way but the most annoying one is that instead of starting a table with a configuration parameter, this is tricking it into executing the table_procexec and using the configuration path as the path to the “new” table, which means that you have no way to provide the new table with a configuration for itself. Gilles > On 8 Jun 2021, at 23:04, Aisha Tammy wrote: > > Hi, > I've attached a slightly updated patch for the procexec. > Ping for someone to take a look :) > > Cheers, > Aisha > > diff --git a/usr.sbin/smtpd/smtpctl/Makefile b/usr.sbin/smtpd/smtpctl/Makefile > index ef8148be8c9..2e8beff1ad1 100644 > --- a/usr.sbin/smtpd/smtpctl/Makefile > +++ b/usr.sbin/smtpd/smtpctl/Makefile > @@ -48,6 +48,7 @@ SRCS+= table_static.c > SRCS+=table_db.c > SRCS+=table_getpwnam.c > SRCS+=table_proc.c > +SRCS+= table_procexec.c > SRCS+=unpack_dns.c > SRCS+=spfwalk.c > > diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h > index be934112103..221f24fbdc4 100644 > --- a/usr.sbin/smtpd/smtpd.h > +++ b/usr.sbin/smtpd/smtpd.h > @@ -1656,6 +1656,7 @@ int table_regex_match(const char *, const char *); > void table_open_all(struct smtpd *); > void table_dump_all(struct smtpd *); > void table_close_all(struct smtpd *); > +const char *table_service_name(enum table_service ); > > > /* to.c */ > diff --git a/usr.sbin/smtpd/smtpd/Makefile b/usr.sbin/smtpd/smtpd/Makefile > index b31d4e42224..debfc8d8ab7 100644 > --- a/usr.sbin/smtpd/smtpd/Makefile > +++ b/usr.sbin/smtpd/smtpd/Makefile > @@ -63,6 +63,7 @@ SRCS+= compress_gzip.c > SRCS+=table_db.c > SRCS+=table_getpwnam.c > SRCS+=table_proc.c > +SRCS+= table_procexec.c > SRCS+=table_static.c > > SRCS+=queue_fs.c > diff --git a/usr.sbin/smtpd/table.c b/usr.sbin/smtpd/table.c > index 1d82d88b81a..0c67d205065 100644 > --- a/usr.sbin/smtpd/table.c > +++ b/usr.sbin/smtpd/table.c > @@ -46,8 +46,8 @@ extern struct table_backend table_backend_static; > extern struct table_backend table_backend_db; > extern struct table_backend table_backend_getpwnam; > extern struct table_backend table_backend_proc; > +extern struct table_backend table_backend_procexec; > > -static const char * table_service_name(enum table_service); > static int table_parse_lookup(enum table_service, const char *, const char *, > union lookup *); > static int parse_sockaddr(struct sockaddr *, int, const char *); > @@ -59,6 +59,7 @@ static struct table_backend *backends[] = { > _backend_db, > _backend_getpwnam, > _backend_proc, > + _backend_procexec, > NULL > }; > > @@ -77,7 +78,7 @@ table_backend_lookup(const char *backend) > return NULL; > } > > -static const char * > +const char * > table_service_name(enum table_service s) > { > switch (s) { > diff --git a/usr.sbin/smtpd/table_procexec.c b/usr.sbin/smtpd/table_procexec.c > new file mode 100644 > index 000..88bfc435fb3 > --- /dev/null > +++ b/usr.sbin/smtpd/table_procexec.c > @@ -0,0 +1,346 @@ > +/* > + * Copyright (c) 2013 Eric Faurot > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > +#include > +#include > +#include > +#include > + > +#include >
Re: Larger kernel fonts in RAMDISK_CD?
On Sun, Jun 06, 2021 at 10:42:59PM +0200, Frederic Cambus wrote: > On Wed, Jun 02, 2021 at 07:56:07PM -0600, Theo de Raadt wrote: > > I am satisfied. > > > > That is one architecture. I suggest checking which others can use > > the same treatment. > > I was going to build i386 releases, but then realized we don't have > efifb on this platform, so we don't need the fonts on RAMDISKs. > > This leaves us with armv7 and arm64. Unfortunately I currently do not > have access to hardware to build releases on, so I would need help on > this front. Anyone willing to try building either version 1) or 2) of > the diffs below (depending if we decide to add only the 16x32 font or > both the 12x24 and 16x32)? Thanks in advance. Here is an update on this, I've been kindly given access (thanks!) to an arm64 system and successfully built a release with the diff below adding both the 12x24 and 16x32 versions. Result of running df -h on the mounted miniroot image produced: /dev/vnd0a18.9M 16.7M2.3M88%/mnt Let's skip armv7 for now, very few of the armv7 hardware we support even has video output anyway. So I'm now asking for OKs for the following diff taking care of amd64 and arm64: Index: sys/arch/amd64/conf/RAMDISK_CD === RCS file: /cvs/src/sys/arch/amd64/conf/RAMDISK_CD,v retrieving revision 1.190 diff -u -p -r1.190 RAMDISK_CD --- sys/arch/amd64/conf/RAMDISK_CD 27 Dec 2020 23:05:37 - 1.190 +++ sys/arch/amd64/conf/RAMDISK_CD 5 Jun 2021 21:45:58 - @@ -20,6 +20,10 @@ option MSDOSFS option INET6 option CRYPTO +option FONT_SPLEEN8x16 +option FONT_SPLEEN12x24 +option FONT_SPLEEN16x32 + option RAMDISK_HOOKS option MINIROOTSIZE=7360 Index: sys/arch/arm64/conf/RAMDISK === RCS file: /cvs/src/sys/arch/arm64/conf/RAMDISK,v retrieving revision 1.153 diff -u -p -r1.153 RAMDISK --- sys/arch/arm64/conf/RAMDISK 29 May 2021 08:50:07 - 1.153 +++ sys/arch/arm64/conf/RAMDISK 5 Jun 2021 21:45:58 - @@ -9,6 +9,10 @@ option SMALL_KERNEL option NO_PROPOLICE option BOOT_CONFIG +option FONT_SPLEEN8x16 +option FONT_SPLEEN12x24 +option FONT_SPLEEN16x32 + option RAMDISK_HOOKS option MINIROOTSIZE=24576 Comments? OK?
Re: patch: unveil: remove some leftover of UNVEIL_INSPECT usage with ni_unveil
On Fri, Mar 12, 2021 at 08:41:59AM +0100, Sebastien Marie wrote: > Hi, > > The following diff is a cleanup to remove two leftover checks, which > were used when ni_unveil was used with UNVEIL_INSPECT: > > it was used by: > - readlink(2) - removed 2019-08-31 > > Make readlink require UNVEIL_READ instead of UNVEIL_INSPECT only > since realpath() is now a system call > > - stat(2) and access(2) - removed 2019-03-24 > > Make stat(2) and access(2) need UNVEIL_READ instead of UNVEIL_INSPECT > > UNVEIL_INSPECT is a hack we added to get chrome/glib working. It silently > adds permission for stat(2), access(2), and readlink(2) to be used on > all path components of any unveil'ed path. robert@ has sucessfully now > fixed chrome/glib to not require exessive TOC vs TOU stat(2) and access(2) > calls on the paths it uses, so that this no longer needed there. > > readlink(2) is the sole call that is now permitted by UNVEIL_INSPECT, > and this is only needed so that realpath(3) can work. Going forward we will > likely make a realpath(2), after which we can completely deprecate > UNVEIL_INSPECT. > > > I audited the values sets in ni_unveil, and UNVEIL_INSPECT is > effectively not used anywhere in this variable. > > The diff removes two checks that were done: > - one in unveil_flagmatch(), for a debug printf > - one in pledge_namei(), for "getpw" usage when using > access("/var/run/ypbind.lock") > > Comments or OK ? OK claudio@ I think all of UNVEIL_INSPECT can go. I have a diff that does that on top of this one. > -- > Sebastien Marie > > diff 48cf7af2deddb13b1f53f18782fd5612c3fdc34a /home/semarie/repos/openbsd/src > blob - 2de0d500e39367046a93c951aeded70bcdeb097d > file + sys/kern/kern_pledge.c > --- sys/kern/kern_pledge.c > +++ sys/kern/kern_pledge.c > @@ -619,8 +619,7 @@ pledge_namei(struct proc *p, struct nameidata *ni, cha > /* when avoiding YP mode, getpw* functions touch this */ > if (ni->ni_pledge == PLEDGE_RPATH && > strcmp(path, "/var/run/ypbind.lock") == 0) { > - if ((p->p_p->ps_pledge & PLEDGE_GETPW) || > - (ni->ni_unveil == UNVEIL_INSPECT)) { > + if (p->p_p->ps_pledge & PLEDGE_GETPW) { > ni->ni_cnd.cn_flags |= BYPASSUNVEIL; > return (0); > } else > blob - 0822248e435b45baf4fa2640cc1a89d85f632cad > file + sys/kern/kern_unveil.c > --- sys/kern/kern_unveil.c > +++ sys/kern/kern_unveil.c > @@ -720,11 +720,6 @@ unveil_flagmatch(struct nameidata *ni, u_char flags) > return 0; > } > } > - if (ni->ni_unveil & UNVEIL_INSPECT) { > -#ifdef DEBUG_UNVEIL > - printf("any unveil allows UNVEIL_INSPECT\n"); > -#endif > - } > return 1; > } > > -- :wq Claudio
Re: setitimer(2): don't round up it_value
On Thu, May 27, 2021 at 06:29:04PM -0500, Scott Cheloha wrote: > On Wed, May 19, 2021 at 10:32:55AM -0500, Scott Cheloha wrote: > > On Wed, May 12, 2021 at 01:15:05PM -0500, Scott Cheloha wrote: > > > > > > [...] > > > > > > Paul de Weerd mentioned off-list that the initial expiration for an > > > ITIMER_REAL timer is always at least one tick. I looked into it and > > > yes, this is the case, because the kernel rounds it_value up to one > > > tick if it is non-zero. > > > > > > After thinking about it a bit I don't think we should do this > > > rounding. At least, not for the initial expiration. > > > > > > [...] > > > > > > Currently the rounding is done in itimerfix(), which takes a timeval > > > pointer as argument. Given that itimerfix() is used nowhere else in > > > the kernel I think the easiest thing to do here is to rewrite > > > itimerfix() to take an itimerval pointer as argument and have it do > > > all input validation and normalization for setitimer(2) in one go: > > > > > > - Validate it_value, return EINVAL if not. > > > > > > - Validate it_interval, return EINVAL if not. > > > > > > - Clear it_interval if it_value is unset. > > > > > > - Round it_interval up if necessary. > > > > > > The 100 million second upper bound for it_value and it_interval is > > > arbitrary and will probably change in the future, so I have isolated > > > that check from the others. > > > > > > While we're changing the itimerfix() prototype we may as well pull it > > > out of sys/time.h. As I said before, it isn't used anywhere else. > > > > > > OK? > > > > Ping. > > 2 week bump. A few comments below. > Index: kern/kern_time.c > === > RCS file: /cvs/src/sys/kern/kern_time.c,v > retrieving revision 1.151 > diff -u -p -r1.151 kern_time.c > --- kern/kern_time.c 23 Dec 2020 20:45:02 - 1.151 > +++ kern/kern_time.c 27 May 2021 23:28:20 - > @@ -52,6 +52,8 @@ > > #include > > +int itimerfix(struct itimerval *); > + > /* > * Time of day and interval timer support. > * > @@ -628,10 +630,9 @@ sys_setitimer(struct proc *p, void *v, r > error = copyin(SCARG(uap, itv), , sizeof(aitv)); > if (error) > return error; > - if (itimerfix(_value) || itimerfix(_interval)) > - return EINVAL; > - if (!timerisset(_value)) > - timerclear(_interval); > + error = itimerfix(); > + if (error) > + return error; > newitvp = > } > if (SCARG(uap, oitv) != NULL) { > @@ -701,21 +702,34 @@ out: > } > > /* > - * Check that a proposed value to load into the .it_value or > - * .it_interval part of an interval timer is acceptable. > + * Check if the given setitimer(2) input is valid. Clear it_interval > + * if it_value is unset. Round it_interval up to the minimum interval > + * if necessary. > */ > int > -itimerfix(struct timeval *tv) > +itimerfix(struct itimerval *itv) > { > + struct timeval min_interval = { .tv_sec = 0, .tv_usec = tick }; > > - if (tv->tv_sec < 0 || tv->tv_sec > 1 || > - tv->tv_usec < 0 || tv->tv_usec >= 100) > - return (EINVAL); > + if (itv->it_value.tv_sec < 0 || !timerisvalid(>it_value)) > + return EINVAL; > + if (itv->it_value.tv_sec > 1) > + return EINVAL; I would prefer this is written as: if (itv->it_value.tv_sec < 0 || itv->it_value.tv_sec > 1) return EINVAL; if (!timerisvalid(>it_value)) return EINVAL; At least comparing seconds together and then doing the usec check seems more logical. > > - if (tv->tv_sec == 0 && tv->tv_usec != 0 && tv->tv_usec < tick) > - tv->tv_usec = tick; > + if (itv->it_interval.tv_sec < 0 || !timerisvalid(>it_interval)) > + return EINVAL; > + if (itv->it_interval.tv_sec > 1) > + return EINVAL; Same as above. > - return (0); > + if (!timerisset(>it_value)) > + timerclear(>it_interval); > + > + if (timerisset(>it_interval)) { > + if (timercmp(>it_interval, _interval, <)) > + itv->it_interval = min_interval; > + } > + > + return 0; > } > > /* > Index: sys/time.h > === > RCS file: /cvs/src/sys/sys/time.h,v > retrieving revision 1.58 > diff -u -p -r1.58 time.h > --- sys/time.h13 Jan 2021 16:28:50 - 1.58 > +++ sys/time.h27 May 2021 23:28:20 - > @@ -307,7 +307,6 @@ struct proc; > int clock_gettime(struct proc *, clockid_t, struct timespec *); > > void cancel_all_itimers(void); > -int itimerfix(struct timeval *); > int itimerdecr(struct itimerspec *, long); > int settime(const struct timespec *); > int ratecheck(struct timeval *, const
Re: smtpd: includes cleanup
Hi. Slightly updated diff, including sys/tree.h in smtpd.h. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.78 diff -u -p -r1.78 aliases.c --- aliases.c 28 Apr 2020 21:46:43 - 1.78 +++ aliases.c 26 May 2021 20:15:02 - @@ -16,19 +16,8 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include #include -#include #include #include "smtpd.h" Index: bounce.c === RCS file: /cvs/src/usr.sbin/smtpd/bounce.c,v retrieving revision 1.84 diff -u -p -r1.84 bounce.c --- bounce.c26 May 2021 18:08:55 - 1.84 +++ bounce.c26 May 2021 20:14:41 - @@ -18,23 +18,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - #include -#include -#include #include -#include -#include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: ca.c === RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v retrieving revision 1.39 diff -u -p -r1.39 ca.c --- ca.c26 May 2021 18:08:55 - 1.39 +++ ca.c27 May 2021 10:57:24 - @@ -17,26 +17,12 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include - -#include -#include +#include +#include #include #include -#include #include #include - -#include -#include -#include -#include -#include -#include -#include #include "smtpd.h" #include "log.h" Index: compress_backend.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_backend.c,v retrieving revision 1.9 diff -u -p -r1.9 compress_backend.c --- compress_backend.c 20 Jan 2015 17:37:54 - 1.9 +++ compress_backend.c 26 May 2021 20:13:39 - @@ -17,18 +17,7 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include #include -#include -#include #include "smtpd.h" Index: compress_gzip.c === RCS file: /cvs/src/usr.sbin/smtpd/compress_gzip.c,v retrieving revision 1.12 diff -u -p -r1.12 compress_gzip.c --- compress_gzip.c 26 May 2021 18:08:55 - 1.12 +++ compress_gzip.c 27 May 2021 11:01:41 - @@ -17,27 +17,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include #include -#include -#include -#include - #include #include "smtpd.h" -#include "log.h" - #defineGZIP_BUFFER_SIZE16384 Index: config.c === RCS file: /cvs/src/usr.sbin/smtpd/config.c,v retrieving revision 1.56 diff -u -p -r1.56 config.c --- config.c26 May 2021 07:05:50 - 1.56 +++ config.c26 May 2021 20:13:14 - @@ -16,23 +16,11 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include -#include #include -#include #include -#include -#include -#include #include -#include #include -#include - -#include #include "smtpd.h" #include "log.h" Index: control.c === RCS file: /cvs/src/usr.sbin/smtpd/control.c,v retrieving revision 1.127 diff -u -p -r1.127 control.c --- control.c 26 May 2021 18:08:55 - 1.127 +++ control.c 26 May 2021 20:12:43 - @@ -18,25 +18,15 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include -#include -#include #include -#include #include #include -#include -#include -#include #include #include -#include #include #include -#include #include -#include #include "smtpd.h" #include "log.h" Index: crypto.c === RCS file: /cvs/src/usr.sbin/smtpd/crypto.c,v retrieving revision 1.9 diff -u -p -r1.9 crypto.c --- crypto.c23 Jan 2021 16:11:11 - 1.9 +++ crypto.c27 May 2021 11:01:30 - @@ -16,14 +16,10 @@ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include -#include - #include - +#include #defineCRYPTO_BUFFER_SIZE 16384 Index: dict.c === RCS file: /cvs/src/usr.sbin/smtpd/dict.c,v retrieving revision 1.7 diff -u -p -r1.7 dict.c --- dict.c 26 May 2021 18:08:55 - 1.7 +++ dict.c 26 May 2021 20:10:54