Re: acpi interrupt storm on HP EliteBook 840 G1
I searched a bit more and found a few cases when other people reported
this behavior:
https://marc.info/?l=openbsd-misc&m=154348280502809&w=2
https://marc.info/?l=openbsd-bugs&m=152022260714390&w=2
I applied a variation of the Thomas Merkel's patch (at the bottom)
which took care of the symptoms while also reporting the masked event.
I am facing a choice. This appears to be a rarely reported problem. I
don't see a good reason to build any non-trivial mitigation (some
variation of event rate limiting comes to mind). I can just carry a
minimized patch forward for as long as I use the defective machine and
rebuild GENERIC.MP as part of my weekly sysupgrade ritual.
Anybody see a reason to prefer complicating the kernel? Will it stand a
chance of getting committed? Any design ideas for such a workaround?
Thanks
Greg
diff --git a/sys/dev/acpi/acpi.c b/sys/dev/acpi/acpi.c
index c3871e007a3..9c069fd5314 100644
--- a/sys/dev/acpi/acpi.c
+++ b/sys/dev/acpi/acpi.c
@@ -2296,6 +2296,15 @@ acpi_gpe(struct acpi_softc *sc, int gpe, void *arg)
struct aml_node *node = arg;
uint8_t mask, en;
+ if (gpe == 0x2a && (sc->gpe_table[gpe].flags & GPE_LEVEL)) {
+ static unsigned short i;
+ if (i == 0) {
+ i++;
+ printf("acpi_gpe %d %s IGNORING\n", gpe, node->name);
+ }
+ return (0);
+ }
+ printf("acpi_gpe %d %s\n", gpe, node->name);
dnprintf(10, "handling GPE %.2x\n", gpe);
aml_evalnode(sc, node, 0, NULL, NULL);
Re: [patch] Check for -1 explicitly in getpeereid.c
Patrick Wildt wrote: > I don't know userland very well, so I have a question. In the middle of > 2019 there have been plenty of changes in regards to changing checks of > syscalls from < 0 to a more strict == -1, like this one in isakmpd: > > > revision 1.26 > date: 2019/06/28 13:32:44; author: deraadt; state: Exp; lines: +2 -2; > commitid: JJ6Ck4WTrgUiEjJp; > When system calls indicate an error they return -1, not some arbitrary > value < 0. errno is only updated in this case. Change all (most?) > callers of syscalls to follow this better, and let's see if this strictness > helps us in the future. > I have about 4000 more changes like that, but I'm stuck with trying to push it further forward. For various reasons, some of which can be guessed from this thread. > getsockopt(), I think, is also a system call. And the manpage indicates > that a failure is always -1, and not some arbitrary number: > > RETURN VALUES > Upon successful completion, the value 0 is returned; otherwise the > value -1 is returned and the global variable errno is set to indicate the > error. > > What is the difference between the diff in this mail, and the changes > done in the middle of last year? The difference is this is direct checking of the syscalls. Versus checking at a higher layer of abstraction, or conversion of that result to something else. Say you have an interface which returns precisely 0 and -1 for two conditions. Well then it has a large set of out-of-range values which (a) won't occur but (b) if they occur, how do you handle them? At which layer? The range of numbers returned really express 3 conditions. One which is impossible, yet if it happens, do you want to convert the impossible to success, or to failure? In the recently supplied diff, a return value of 50 at the system call layer, is returned into a library returning 0 (success). Furthermore, the diff itself proposes treating the out-of-range impossible as a success, and accesses memory which is very probably unintialized. > getsockopt() isn't allowed to return > anything else but 0 and 1, right? Though I guess the current check > (error != 0) is the one that also catches instances where getsockopt() > isn't behaving well, even though it shouldn't. But then, with the -1 > check, wouldn't we be catching more instances of syscalls misbehaving > if we checked for < -1? Correct. I hope you have reached the same indecision point as me. I feel uncomfortable changing all checking-points to 3-way decision. And imagine what a modern compiler would do there...
Re: [patch] Check for -1 explicitly in getpeereid.c
Hi, I don't know userland very well, so I have a question. In the middle of 2019 there have been plenty of changes in regards to changing checks of syscalls from < 0 to a more strict == -1, like this one in isakmpd: revision 1.26 date: 2019/06/28 13:32:44; author: deraadt; state: Exp; lines: +2 -2; commitid: JJ6Ck4WTrgUiEjJp; When system calls indicate an error they return -1, not some arbitrary value < 0. errno is only updated in this case. Change all (most?) callers of syscalls to follow this better, and let's see if this strictness helps us in the future. getsockopt(), I think, is also a system call. And the manpage indicates that a failure is always -1, and not some arbitrary number: RETURN VALUES Upon successful completion, the value 0 is returned; otherwise the value -1 is returned and the global variable errno is set to indicate the error. What is the difference between the diff in this mail, and the changes done in the middle of last year? getsockopt() isn't allowed to return anything else but 0 and 1, right? Though I guess the current check (error != 0) is the one that also catches instances where getsockopt() isn't behaving well, even though it shouldn't. But then, with the -1 check, wouldn't we be catching more instances of syscalls misbehaving if we checked for < -1? Patrick On Sun, Apr 26, 2020 at 02:45:54PM -0600, Theo de Raadt wrote: > If it returns 50 then the creds structure is not valid, and can't be copied > from. It only returns valid creds *IF* success is indicated by 0. But then > you convert 50 to a return value of 0, and hide any indication that things > went weird? > > No way, I'm not buying your argument. I think the code is making the > correct choice now. > > Martin Vahlensieck wrote: > > > Hi there > > > > From the getsockopt(2) manual page says getsockopt(2) returns -1 on > > error and 0 on success. Also getpeereid(3) only lists those 2 values. > > This diff makes the return value check in getpeereid explicit. I guess > > this is how it is done elsewhere in the tree (there is a commit turning > > a bunch of "... < 0" to "== -1" I think this falls under that category). > > > > Best, > > > > Martin > > > > Index: net/getpeereid.c > > === > > RCS file: /cvs/src/lib/libc/net/getpeereid.c,v > > retrieving revision 1.1 > > diff -u -p -r1.1 getpeereid.c > > --- net/getpeereid.c1 Jul 2010 19:15:30 - 1.1 > > +++ net/getpeereid.c26 Apr 2020 20:28:50 - > > @@ -28,7 +28,7 @@ getpeereid(int s, uid_t *euid, gid_t *eg > > > > error = getsockopt(s, SOL_SOCKET, SO_PEERCRED, > > &creds, &credslen); > > - if (error) > > + if (error == -1) > > return (error); > > *euid = creds.uid; > > *egid = creds.gid; > > >
Re: [patch] Check for -1 explicitly in getpeereid.c
If it returns 50 then the creds structure is not valid, and can't be copied from. It only returns valid creds *IF* success is indicated by 0. But then you convert 50 to a return value of 0, and hide any indication that things went weird? No way, I'm not buying your argument. I think the code is making the correct choice now. Martin Vahlensieck wrote: > Hi there > > From the getsockopt(2) manual page says getsockopt(2) returns -1 on > error and 0 on success. Also getpeereid(3) only lists those 2 values. > This diff makes the return value check in getpeereid explicit. I guess > this is how it is done elsewhere in the tree (there is a commit turning > a bunch of "... < 0" to "== -1" I think this falls under that category). > > Best, > > Martin > > Index: net/getpeereid.c > === > RCS file: /cvs/src/lib/libc/net/getpeereid.c,v > retrieving revision 1.1 > diff -u -p -r1.1 getpeereid.c > --- net/getpeereid.c 1 Jul 2010 19:15:30 - 1.1 > +++ net/getpeereid.c 26 Apr 2020 20:28:50 - > @@ -28,7 +28,7 @@ getpeereid(int s, uid_t *euid, gid_t *eg > > error = getsockopt(s, SOL_SOCKET, SO_PEERCRED, > &creds, &credslen); > - if (error) > + if (error == -1) > return (error); > *euid = creds.uid; > *egid = creds.gid; >
[patch] Check for -1 explicitly in getpeereid.c
Hi there >From the getsockopt(2) manual page says getsockopt(2) returns -1 on error and 0 on success. Also getpeereid(3) only lists those 2 values. This diff makes the return value check in getpeereid explicit. I guess this is how it is done elsewhere in the tree (there is a commit turning a bunch of "... < 0" to "== -1" I think this falls under that category). Best, Martin Index: net/getpeereid.c === RCS file: /cvs/src/lib/libc/net/getpeereid.c,v retrieving revision 1.1 diff -u -p -r1.1 getpeereid.c --- net/getpeereid.c1 Jul 2010 19:15:30 - 1.1 +++ net/getpeereid.c26 Apr 2020 20:28:50 - @@ -28,7 +28,7 @@ getpeereid(int s, uid_t *euid, gid_t *eg error = getsockopt(s, SOL_SOCKET, SO_PEERCRED, &creds, &credslen); - if (error) + if (error == -1) return (error); *euid = creds.uid; *egid = creds.gid;
Re: coherent em(4) descriptor rings (to be able to run on my arm64 machine)
> Date: Sun, 26 Apr 2020 18:03:20 +0200 > From: Patrick Wildt > > Hi, > > I have a HummingBoard Pulse, which is an NXP i.MX8MQ based board > featuring two ethernets, with the second ethernet being an em(4) on > a PCIe controller. > > I had trouble getting it to work, but realized that the issue is the > descriptor ring coherency. I looked into the code, tried to find if > there are incorrect flushes, or something else, but nothing worked. > > Looking at the Linux driver I realized that their descriptor rings are > allocated coherent. Some arm64 machines are fine with em(4), since the > PCIe controller is coherent, but on my machine it is not. Explicitly > mapping the rings coherent made my machine happy, so I believe that > maybe the way that em(4) works, we need to make sure the rings are > coherent. > > So I'd propose the following diff, which *only* makes the desciptor > rings coherent, the packets stay cached and fast. This allows me to > push plenty of traffic through my machine! > > This is a no-op on all x86, and on arm64-machines with coherent PCIe > controllers. > > Opinions? ok? In principle the BUS_DMAMAP_COHERENT flag should not matter; things should work either way. This indicates there is something wrong in the code here, either a missing bus_dmamap_sync() call, or a bug in the arm64 bus_dmamp_sync() implementation. Things are always tricky for descriptor rings, where software and hardware are modifying the memory contents. Things get especially tricky when a cache line covers multiple ring entries. Given that ARMv8 typically has a cache line size of 64 bytes and em(4) has descriptors of 16 bytes, that may very well be a problem here. That said. BUS_DMA_COHERENT makes a lot of sense for descriptor rings like this since otherwise you just end up doing doing lots of cache flushes. And I think you've spent enough time looking for the real bug. Quickly tested on the Ampere machine. ok kettenis@ > diff --git a/sys/dev/pci/if_em.c b/sys/dev/pci/if_em.c > index aca6b4bb02f..27d0630bf9f 100644 > --- a/sys/dev/pci/if_em.c > +++ b/sys/dev/pci/if_em.c > @@ -2113,7 +2113,7 @@ em_dma_malloc(struct em_softc *sc, bus_size_t size, > struct em_dma_alloc *dma) > goto destroy; > > r = bus_dmamem_map(sc->sc_dmat, &dma->dma_seg, dma->dma_nseg, size, > - &dma->dma_vaddr, BUS_DMA_WAITOK); > + &dma->dma_vaddr, BUS_DMA_WAITOK | BUS_DMA_COHERENT); > if (r != 0) > goto free; > > >
smtpd: fix catch-all in virtual aliases
Hi. When a catch-all entry (@) is used in a virtual alias table, it eventually (and mistakenly) catches everything that expands to a username. For example, with: f...@example.com user @catchall "f...@example.com" expands to "user" as expected, but then "user" expands to "catchall" because it is interpreted as "user@" (empty domain). The catch-all fallback mechanism is really meant for full email addresses in virtual context, and should not happen for usernames. The following diff fixes it. Eric. Index: aliases.c === RCS file: /cvs/src/usr.sbin/smtpd/aliases.c,v retrieving revision 1.77 diff -u -p -r1.77 aliases.c --- aliases.c 28 Dec 2018 12:47:28 - 1.77 +++ aliases.c 26 Apr 2020 16:04:51 - @@ -164,6 +164,10 @@ aliases_virtual_get(struct expand *expan if (ret) goto expand; + /* Do not try catch-all entries if there is no domain */ + if (domain[0] == '\0') + return 0; + if (!bsnprintf(buf, sizeof(buf), "@%s", domain)) return 0; /* Failed ? We lookup for catch all for virtual domain */
coherent em(4) descriptor rings (to be able to run on my arm64 machine)
Hi, I have a HummingBoard Pulse, which is an NXP i.MX8MQ based board featuring two ethernets, with the second ethernet being an em(4) on a PCIe controller. I had trouble getting it to work, but realized that the issue is the descriptor ring coherency. I looked into the code, tried to find if there are incorrect flushes, or something else, but nothing worked. Looking at the Linux driver I realized that their descriptor rings are allocated coherent. Some arm64 machines are fine with em(4), since the PCIe controller is coherent, but on my machine it is not. Explicitly mapping the rings coherent made my machine happy, so I believe that maybe the way that em(4) works, we need to make sure the rings are coherent. So I'd propose the following diff, which *only* makes the desciptor rings coherent, the packets stay cached and fast. This allows me to push plenty of traffic through my machine! This is a no-op on all x86, and on arm64-machines with coherent PCIe controllers. Opinions? ok? Patrick diff --git a/sys/dev/pci/if_em.c b/sys/dev/pci/if_em.c index aca6b4bb02f..27d0630bf9f 100644 --- a/sys/dev/pci/if_em.c +++ b/sys/dev/pci/if_em.c @@ -2113,7 +2113,7 @@ em_dma_malloc(struct em_softc *sc, bus_size_t size, struct em_dma_alloc *dma) goto destroy; r = bus_dmamem_map(sc->sc_dmat, &dma->dma_seg, dma->dma_nseg, size, - &dma->dma_vaddr, BUS_DMA_WAITOK); + &dma->dma_vaddr, BUS_DMA_WAITOK | BUS_DMA_COHERENT); if (r != 0) goto free;
Re: correction for insque.3
On Thu, Apr 23, 2020 at 05:48:42PM -0400, Andras Farkas wrote:
> I was reading the thread about STAILQ and SIMPLEQ and thought it was
> interesting, so I then read a little about sys/queue.h and search.h
> I noticed an error in insque.3:
> https://man.openbsd.org/insque.3
> The words "next" and "previous" are swapped, as it is the first
> pointer that points to the next element, and the second pointer that
> points to the previous element.
> I confirmed this both in insque.c (to make sure I understood the
> directionality correctly):
> https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/insque.c?rev=1.3&content-type=text/x-cvsweb-markup
> and in POSIX, which the man page states OpenBSD's insque() conforms to:
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/insque.html
>
> I have the diff to fix this both inline and attached.
> Checksum for anyone trying to grab the diff from inline, since my
> inline diffs usually get mangled:
> SHA256 (insque3diff.txt) =
> 8340d135c06abc46127a12307b9611d274e1ad3e35dc6cf662da15a1f4502dec
>
> Index: insque.3
> ===
> RCS file: /cvs/src/lib/libc/stdlib/insque.3,v
> retrieving revision 1.10
> diff -u -p -r1.10 insque.3
> --- insque.3 30 Nov 2014 20:19:12 - 1.10
> +++ insque.3 23 Apr 2020 21:13:10 -
> @@ -61,7 +61,7 @@ struct qelem {
> .Ed
> .Pp
> The first two elements in the struct must be pointers of the
> -same type that point to the previous and next elements in
> +same type that point to the next and previous elements in
> the queue respectively.
> Any subsequent data in the struct is application-dependent.
> .Pp
>
hi. i just committed this part.
> Also, I feel the phrasing isn't as clear as it could be. The way
> "respectively" is used makes more sense in the way the other BSD man
> pages use "first and second" (in "first and second members") rather
> than the OpenBSD man page's "first two" (in "first two elements"). I
> think "respectively" makes more sense when used in analogy, or pairing
> up words.
> https://www.freebsd.org/cgi/man.cgi?query=insque&apropos=0&sektion=0&manpath=FreeBSD+12.1-RELEASE+and+Ports&arch=default&format=html
> https://netbsd.gw.com/cgi-bin/man-cgi?insque
> https://leaf.dragonflybsd.org/cgi/web-man?command=insque§ion=ANY
> Their wording is identical.
>
but could;t convince myself that this was any clearer.
jmc
> I have a larger diff as an option, in light of the above:
> SHA256 (insque3difflarge.txt) =
> c2e242221f016ae4bafa3b40a466f27016d4f44b3f58053a60204e06aecefaf9
>
> Index: insque.3
> ===
> RCS file: /cvs/src/lib/libc/stdlib/insque.3,v
> retrieving revision 1.10
> diff -u -p -r1.10 insque.3
> --- insque.3 30 Nov 2014 20:19:12 - 1.10
> +++ insque.3 23 Apr 2020 21:42:07 -
> @@ -60,8 +60,8 @@ struct qelem {
> };
> .Ed
> .Pp
> -The first two elements in the struct must be pointers of the
> -same type that point to the previous and next elements in
> +The first and second elements in the struct must be pointers of the
> +same type that point to the next and previous elements in
> the queue respectively.
> Any subsequent data in the struct is application-dependent.
> .Pp
>
> Thanks for reading!
> Index: insque.3
> ===
> RCS file: /cvs/src/lib/libc/stdlib/insque.3,v
> retrieving revision 1.10
> diff -u -p -r1.10 insque.3
> --- insque.3 30 Nov 2014 20:19:12 - 1.10
> +++ insque.3 23 Apr 2020 21:13:10 -
> @@ -61,7 +61,7 @@ struct qelem {
> .Ed
> .Pp
> The first two elements in the struct must be pointers of the
> -same type that point to the previous and next elements in
> +same type that point to the next and previous elements in
> the queue respectively.
> Any subsequent data in the struct is application-dependent.
> .Pp
> Index: insque.3
> ===
> RCS file: /cvs/src/lib/libc/stdlib/insque.3,v
> retrieving revision 1.10
> diff -u -p -r1.10 insque.3
> --- insque.3 30 Nov 2014 20:19:12 - 1.10
> +++ insque.3 23 Apr 2020 21:42:07 -
> @@ -60,8 +60,8 @@ struct qelem {
> };
> .Ed
> .Pp
> -The first two elements in the struct must be pointers of the
> -same type that point to the previous and next elements in
> +The first and second elements in the struct must be pointers of the
> +same type that point to the next and previous elements in
> the queue respectively.
> Any subsequent data in the struct is application-dependent.
> .Pp
Raspberry Pi GPIO
Diff below adds GPIO support to bcmgpio(4). It also adds the bits to
attach gpio(4) such that GPIO pins can be controlled from userland.
This makes sense on boards like the Raspberry Pi and the
implementation makes sure that pins used by kernel drivers can't be
touched.
ok?
Index: arch/arm64/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/arm64/conf/GENERIC,v
retrieving revision 1.161
diff -u -p -r1.161 GENERIC
--- arch/arm64/conf/GENERIC 25 Apr 2020 22:28:12 - 1.161
+++ arch/arm64/conf/GENERIC 26 Apr 2020 10:51:19 -
@@ -148,6 +148,7 @@ bcmclock* at fdt? early 1
bcmdmac* at fdt? early 1
bcmdog*at fdt?
bcmgpio* at fdt? early 1
+gpio* at bcmgpio?
bcmintc* at fdt? early 1
bcmirng* at fdt?
bcmmbox* at fdt? early 1
Index: dev/fdt/bcm2835_gpio.c
===
RCS file: /cvs/src/sys/dev/fdt/bcm2835_gpio.c,v
retrieving revision 1.2
diff -u -p -r1.2 bcm2835_gpio.c
--- dev/fdt/bcm2835_gpio.c 25 Apr 2020 22:15:00 - 1.2
+++ dev/fdt/bcm2835_gpio.c 26 Apr 2020 10:51:20 -
@@ -17,16 +17,21 @@
#include
#include
+#include
#include
#include
#include
#include
+#include
#include
+#include
#include
#include
+#include "gpio.h"
+
/* Registers */
#define GPFSEL(n) (0x00 + ((n) * 4))
#define GPFSEL_MASK 0x7
@@ -38,6 +43,9 @@
#define GPFSEL_ALT3 0x7
#define GPFSEL_ALT4 0x3
#define GPFSEL_ALT5 0x2
+#define GPSET(n) (0x1c + ((n) * 4))
+#define GPCLR(n) (0x28 + ((n) * 4))
+#define GPLEV(n) (0x34 + ((n) * 4))
#define GPPUD 0x94
#define GPPUD_PUD 0x3
#define GPPUD_PUD_OFF 0x0
@@ -47,6 +55,8 @@
#define GPPULL(n) (0xe4 + ((n) * 4))
#define GPPULL_MASK 0x3
+#define BCMGPIO_MAX_PINS 58
+
#define HREAD4(sc, reg)
\
(bus_space_read_4((sc)->sc_iot, (sc)->sc_ioh, (reg)))
#define HWRITE4(sc, reg, val) \
@@ -58,6 +68,13 @@ struct bcmgpio_softc {
bus_space_handle_t sc_ioh;
void(*sc_config_pull)(struct bcmgpio_softc *, int, int);
+ int sc_num_pins;
+
+ struct gpio_controller sc_gc;
+
+ struct gpio_chipset_tag sc_gpio_tag;
+ gpio_pin_t sc_gpio_pins[BCMGPIO_MAX_PINS];
+ int sc_gpio_claimed[BCMGPIO_MAX_PINS];
};
intbcmgpio_match(struct device *, void *, void *);
@@ -74,6 +91,10 @@ struct cfdriver bcmgpio_cd = {
void bcm2711_config_pull(struct bcmgpio_softc *, int, int);
void bcm2835_config_pull(struct bcmgpio_softc *, int, int);
intbcmgpio_pinctrl(uint32_t, void *);
+void bcmgpio_config_pin(void *, uint32_t *, int);
+intbcmgpio_get_pin(void *, uint32_t *);
+void bcmgpio_set_pin(void *, uint32_t *, int);
+void bcmgpio_attach_gpio(struct device *);
int
bcmgpio_match(struct device *parent, void *match, void *aux)
@@ -104,12 +125,24 @@ bcmgpio_attach(struct device *parent, st
printf("\n");
- if (OF_is_compatible(faa->fa_node, "brcm,bcm2711-gpio"))
+ if (OF_is_compatible(faa->fa_node, "brcm,bcm2711-gpio")) {
sc->sc_config_pull = bcm2711_config_pull;
- else
+ sc->sc_num_pins = 58;
+ } else {
sc->sc_config_pull = bcm2835_config_pull;
+ sc->sc_num_pins = 54;
+ }
pinctrl_register(faa->fa_node, bcmgpio_pinctrl, sc);
+
+ sc->sc_gc.gc_node = faa->fa_node;
+ sc->sc_gc.gc_cookie = sc;
+ sc->sc_gc.gc_config_pin = bcmgpio_config_pin;
+ sc->sc_gc.gc_get_pin = bcmgpio_get_pin;
+ sc->sc_gc.gc_set_pin = bcmgpio_set_pin;
+ gpio_controller_register(&sc->sc_gc);
+
+ config_mountroot(self, bcmgpio_attach_gpio);
}
void
@@ -186,6 +219,7 @@ bcmgpio_pinctrl(uint32_t phandle, void *
bcmgpio_config_func(sc, pins[i], func);
if (plen > 0 && i < plen / sizeof(uint32_t))
sc->sc_config_pull(sc, pins[i], pull[i]);
+ sc->sc_gpio_claimed[pins[i]] = 1;
}
free(pull, M_TEMP, plen);
@@ -196,4 +230,176 @@ fail:
free(pull, M_TEMP, plen);
free(pins, M_TEMP, len);
return -1;
+}
+
+void
+bcmgpio_config_pin(void *cookie, uint32_t *cells, int config)
+{
+ struct bcmgpio_softc *sc = cookie;
+ uint32_t pin = cells[0];
+
+ if (pin >= sc->sc_num_pins)
+ return;
+
+ if (config & GPIO_CONFIG_OUTPUT)
+ bcmgpio_config_func(sc, pin, GPFSEL_GPIO_OUT);
+ else
+ bcmgpio_config_func(sc, pin, GPFSEL_GPIO_OUT);
+ if (config & GPIO_CONFIG_PULL_UP)
+ sc->sc_config_pull(sc, pin, GPPUD_PUD_UP);
+ else
Re: OpenSMTPD: unprivileged mode - now with diff
Hello, Christopher On the right of a person who had successfully run rootless sendmail installations for many years, please find some comments below. On 2020-04-26 12:30, Christopher Zimmermann wrote: Thanks for giving it a thought. I'm not entirely convinced either. But believe some thought should be given to it. In your opinion would it be generaly a bad idea to try run smtpd without root privileges? What exectly will cease to work? - .forward and alias _filtering_ will break for sure. not necessarily. at least as long as users will have smtpd and their .forward and in the same group and you've documented it. also, .forward and authentication could be handled by a separate daemon bound to unix socket only, which won't listen to outside world. or even better, separate all the functions, that usually need root access to such daemon(s). the only thing was broken for me in sendmail's case, was mbox deliveries. but AFAIR, that was solved by having patched version of mail.local. -- With best regards, Gregory Edigarov
Re: OpenSMTPD: unprivileged mode - now with diff
On Sun, Apr 26, 2020 at 08:55:14AM +, gil...@poolp.org wrote: April 26, 2020 10:34 AM, "Christopher Zimmermann" wrote: - always run lmtp deliveries as SMTPD_USER. The change to mda_unpriv.c is needed, because otherwise all mails would be delivered to SMTPD_USER. - add two internal flags NOPRIV and NEEDPRIV. NOPRIV can be configured by the simple directive "no-priv". NEEDPRIV gets set on all delivery methods / options requiring setuid() to run as the receipient user. A configuration error is produced on any conflict betweed NEEDPRIV and NOPRIV. In case of a NOPRIV run smtpd will drop root privileges. This will break .forward and alias filters. Hi, thanks for your fast reply. The LMTP change seems interesting to me, it means that a broken LMTP delivery will fail with _smtpd privileges instead of the (unprivileged) recipient user so I think it's a good move. That's great. Is there anything that needs to change or be clarified before you can give an ok? I'm less convinced by the other change and it doesn't only break .forward and alias, it also breaks authentication, tables reloading, and probably stuff my mind is not yet awake enough to think of. Thanks for giving it a thought. I'm not entirely convinced either. But believe some thought should be given to it. In your opinion would it be generaly a bad idea to try run smtpd without root privileges? What exectly will cease to work? - .forward and alias _filtering_ will break for sure. Forwarding won't break. - Authentication: Authentication of system users will break. So I would mark auth without auth-table as need_priv() in parse.y. - tables reloading: this is a problem only for tables with restricted read permissions, isn't it? I could try to figure out whether it is a problem in parse.y and mark it as need_priv() if necessary, but one could also just document the no-priv option with its limitations. - other stuff: Can it be dealt with need_priv()? Will it lead to unpleasant surprises for users? The general idea is to allow running smtpd without root priviliges where possible and maybe try to change it to support running without root for more use-cases over time. Like try to pick the low-hanging fruits first. One less low-hanging fruits would be to integrate mail.lmtp(8) into smtpd and move other mail.* functionality into a privileged lmtpd daemon. This would remove those vulnerable mda-exec lines from the envelope files. That's just one idea how one could proceed. The current proposal is to start stripping privileges where possible. Christopher -- http://gmerlin.de OpenPGP: http://gmerlin.de/christopher.pub CB07 DA40 B0B6 571D 35E2 0DEF 87E2 92A7 13E5 DEE1
Re: OpenSMTPD: unprivileged mode - now with diff
April 26, 2020 10:34 AM, "Christopher Zimmermann" wrote:
> Hi,
>
> I further developed my approach to allow running smtpd with fewer privileges.
> This diff does two
> things:
>
> - always run lmtp deliveries as SMTPD_USER. The change to mda_unpriv.c is
> needed, because otherwise
> all mails would be delivered to SMTPD_USER.
>
> - add two internal flags NOPRIV and NEEDPRIV. NOPRIV can be configured by the
> simple directive
> "no-priv". NEEDPRIV gets set on all delivery methods / options requiring
> setuid() to run as the
> receipient user.
> A configuration error is produced on any conflict betweed NEEDPRIV and NOPRIV.
> In case of a NOPRIV run smtpd will drop root privileges.
> This will break .forward and alias filters.
>
> The change to the lmtp delivery has benefits even without the second change.
> With the second change
> my smtpd now runs without root privileges.
> The NEEDPRIV/NOPRIV options are meant to allow restricting of the privileges
> of other delivery
> methods.
>
> I am now looking for OKs on the first change to do unprivileged lmtp
> deliveries and feedback on the
> general approach of the second change.
>
The LMTP change seems interesting to me, it means that a broken LMTP delivery
will fail with _smtpd privileges instead of the (unprivileged) recipient user
so I think it's a good move.
I'm less convinced by the other change and it doesn't only break .forward and
alias, it also breaks authentication, tables reloading, and probably stuff my
mind is not yet awake enough to think of.
> Index: mda_unpriv.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mda_unpriv.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 mda_unpriv.c
> --- mda_unpriv.c 2 Feb 2020 22:13:48 - 1.6
> +++ mda_unpriv.c 26 Apr 2020 05:27:34 -
> @@ -69,8 +69,8 @@ mda_unpriv(struct dispatcher *dsp, struc
> xasprintf(&mda_environ[idx++], "RECIPIENT=%s@%s", deliver->dest.user,
> deliver->dest.domain);
> xasprintf(&mda_environ[idx++], "SHELL=/bin/sh");
> xasprintf(&mda_environ[idx++], "LOCAL=%s", deliver->rcpt.user);
> - xasprintf(&mda_environ[idx++], "LOGNAME=%s", pw_name);
> - xasprintf(&mda_environ[idx++], "USER=%s", pw_name);
> + xasprintf(&mda_environ[idx++], "LOGNAME=%s", deliver->userinfo.username);
> + xasprintf(&mda_environ[idx++], "USER=%s", deliver->userinfo.username);
> if (deliver->sender.user[0])
> xasprintf(&mda_environ[idx++], "SENDER=%s@%s",
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> retrieving revision 1.277
> diff -u -p -r1.277 parse.y
> --- parse.y 24 Feb 2020 23:54:27 - 1.277
> +++ parse.y 26 Apr 2020 05:27:35 -
> @@ -154,6 +154,7 @@ static int host_v4(struct listen_opts *)
> static int host_v6(struct listen_opts *);
> static int host_dns(struct listen_opts *);
> static int interface(struct listen_opts *);
> +static void need_priv(const char *);
> int delaytonum(char *);
> int is_if_in_group(const char *, const char *);
> @@ -186,7 +187,7 @@ typedef struct {
> %token KEY
> %token LIMIT LISTEN LMTP LOCAL
> %token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE
> MAX_DEFERRED MBOX MDA MTA MX
> -%token NO_DSN NO_VERIFY NOOP
> +%token NO_DSN NO_PRIV NO_VERIFY NOOP
> %token ON
> %token PHASE PKI PORT PROC PROC_EXEC PROXY_V2
> %token QUEUE QUIT
> @@ -212,6 +213,7 @@ grammar : /* empty */
> | grammar ca '\n'
> | grammar mda '\n'
> | grammar mta '\n'
> + | grammar privs '\n'
> | grammar pki '\n'
> | grammar proc '\n'
> | grammar queue '\n'
> @@ -379,6 +381,20 @@ MTA MAX_DEFERRED NUMBER {
> ;
> +privs:
> +NO_PRIV {
> + if (conf->sc_opts & SMTPD_OPT_NEEDPRIV) {
> + yyerror("Unprivileged operation is not possible.");
> + YYERROR;
> + }
> + else {
> + log_warnx("Unprivileged operation requested.");
> + conf->sc_opts |= SMTPD_OPT_NOPRIV;
> + }
> +}
> +;
> +
> +
> pki:
> PKI STRING {
> char buf[HOST_NAME_MAX+1];
> @@ -566,6 +582,8 @@ SRS KEY STRING {
> dispatcher_local_option:
> USER STRING {
> + need_priv("with user");
> +
> if (dispatcher->u.local.is_mbox) {
> yyerror("user may not be specified for this dispatcher");
> YYERROR;
> @@ -662,16 +680,20 @@ dispatcher_local_option dispatcher_local
> dispatcher_local:
> MBOX {
> + need_priv("mbox");
> dispatcher->u.local.is_mbox = 1;
> asprintf(&dispatcher->u.local.command, "/usr/libexec/mail.local -f
> %%{mbox.from} --
> %%{user.username}");
> } dispatcher_local_options
> | MAILDIR {
> + need_priv("maildir");
> asprintf(&dispatcher->u.local.command, "/usr/libexec/mail.maildir");
> } dispatcher_local_options
> | MAILDIR JUNK {
> + need_priv("maildir");
> asprintf(&dispatcher->u.local.command, "/usr/libexec/mail.maildir -j");
> } dispatcher_local_options
> | MAILDIR STRING {
> + need_priv("maildir");
> if (strncmp($2, "~/", 2) == 0)
> asprintf(&dispatcher->u.local.command,
> "/usr/libexec/mail.maildir \"%%{user.directory}/%s\"", $2+2);
> @@ -680,6 +702,7 @@ MBOX {
> "/usr/libexe
OpenSMTPD: unprivileged mode - now with diff
Hi,
I further developed my approach to allow running smtpd with fewer
privileges. This diff does two things:
- always run lmtp deliveries as SMTPD_USER. The change to mda_unpriv.c
is needed, because otherwise all mails would be delivered to
SMTPD_USER.
- add two internal flags NOPRIV and NEEDPRIV. NOPRIV can be configured
by the simple directive "no-priv". NEEDPRIV gets set on all delivery
methods / options requiring setuid() to run as the receipient user.
A configuration error is produced on any conflict betweed NEEDPRIV and
NOPRIV.
In case of a NOPRIV run smtpd will drop root privileges.
This will break .forward and alias filters.
The change to the lmtp delivery has benefits even without the second
change. With the second change my smtpd now runs without root
privileges.
The NEEDPRIV/NOPRIV options are meant to allow restricting of the
privileges of other delivery methods.
I am now looking for OKs on the first change to do unprivileged lmtp
deliveries and feedback on the general approach of the second change.
Christopher
Index: mda_unpriv.c
===
RCS file: /cvs/src/usr.sbin/smtpd/mda_unpriv.c,v
retrieving revision 1.6
diff -u -p -r1.6 mda_unpriv.c
--- mda_unpriv.c2 Feb 2020 22:13:48 - 1.6
+++ mda_unpriv.c26 Apr 2020 05:27:34 -
@@ -69,8 +69,8 @@ mda_unpriv(struct dispatcher *dsp, struc
xasprintf(&mda_environ[idx++], "RECIPIENT=%s@%s", deliver->dest.user,
deliver->dest.domain);
xasprintf(&mda_environ[idx++], "SHELL=/bin/sh");
xasprintf(&mda_environ[idx++], "LOCAL=%s", deliver->rcpt.user);
- xasprintf(&mda_environ[idx++], "LOGNAME=%s", pw_name);
- xasprintf(&mda_environ[idx++], "USER=%s", pw_name);
+ xasprintf(&mda_environ[idx++], "LOGNAME=%s",
deliver->userinfo.username);
+ xasprintf(&mda_environ[idx++], "USER=%s", deliver->userinfo.username);
if (deliver->sender.user[0])
xasprintf(&mda_environ[idx++], "SENDER=%s@%s",
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.277
diff -u -p -r1.277 parse.y
--- parse.y 24 Feb 2020 23:54:27 - 1.277
+++ parse.y 26 Apr 2020 05:27:35 -
@@ -154,6 +154,7 @@ static int host_v4(struct listen_opts *)
static int host_v6(struct listen_opts *);
static int host_dns(struct listen_opts *);
static int interface(struct listen_opts *);
+static voidneed_priv(const char *);
int delaytonum(char *);
int is_if_in_group(const char *, const char *);
@@ -186,7 +187,7 @@ typedef struct {
%token KEY
%token LIMIT LISTEN LMTP LOCAL
%token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE
MAX_DEFERRED MBOX MDA MTA MX
-%token NO_DSN NO_VERIFY NOOP
+%token NO_DSN NO_PRIV NO_VERIFY NOOP
%token ON
%token PHASE PKI PORT PROC PROC_EXEC PROXY_V2
%token QUEUE QUIT
@@ -212,6 +213,7 @@ grammar : /* empty */
| grammar ca '\n'
| grammar mda '\n'
| grammar mta '\n'
+ | grammar privs '\n'
| grammar pki '\n'
| grammar proc '\n'
| grammar queue '\n'
@@ -379,6 +381,20 @@ MTA MAX_DEFERRED NUMBER {
;
+privs:
+NO_PRIV {
+ if (conf->sc_opts & SMTPD_OPT_NEEDPRIV) {
+ yyerror("Unprivileged operation is not possible.");
+ YYERROR;
+ }
+ else {
+ log_warnx("Unprivileged operation requested.");
+ conf->sc_opts |= SMTPD_OPT_NOPRIV;
+ }
+}
+;
+
+
pki:
PKI STRING {
char buf[HOST_NAME_MAX+1];
@@ -566,6 +582,8 @@ SRS KEY STRING {
dispatcher_local_option:
USER STRING {
+ need_priv("with user");
+
if (dispatcher->u.local.is_mbox) {
yyerror("user may not be specified for this dispatcher");
YYERROR;
@@ -662,16 +680,20 @@ dispatcher_local_option dispatcher_local
dispatcher_local:
MBOX {
+ need_priv("mbox");
dispatcher->u.local.is_mbox = 1;
asprintf(&dispatcher->u.local.command, "/usr/libexec/mail.local -f
%%{mbox.from} -- %%{user.username}");
} dispatcher_local_options
| MAILDIR {
+ need_priv("maildir");
asprintf(&dispatcher->u.local.command, "/usr/libexec/mail.maildir");
} dispatcher_local_options
| MAILDIR JUNK {
+ need_priv("maildir");
asprintf(&dispatcher->u.local.command, "/usr/libexec/mail.maildir -j");
} dispatcher_local_options
| MAILDIR STRING {
+ need_priv("maildir");
if (strncmp($2, "~/", 2) == 0)
asprintf(&dispatcher->u.local.command,
"/usr/libexec/mail.maildir \"%%{user.directory}/%s\"",
$2+2);
@@ -680,6 +702,7 @@ MBOX {
"/usr/libexec/mail.maildir \"%s\"", $2);
} dispatcher_local_options
| MAILDIR STRING JUNK {
+ need_priv("maildir");
