Re: prevent bgpd from starting when control socket already used
Hi,
from the discussion I understand nobody rejects the functionality.
To ease the review here again the diff (with incorporated feedback from
anton@ (redundant parens)).
Any comments or OKs?
Remi
cvs diff: Diffing .
Index: bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.204
diff -u -p -r1.204 bgpd.c
--- bgpd.c 29 Sep 2018 08:11:11 - 1.204
+++ bgpd.c 18 Nov 2018 22:21:51 -
@@ -939,6 +939,8 @@ control_setup(struct bgpd_config *conf)
}
if ((cname = strdup(conf->csock)) == NULL)
fatal("strdup");
+ if (control_check(cname) == -1)
+ fatalx("control socket check failed");
if ((fd = control_init(0, cname)) == -1)
fatalx("control socket setup failed");
if (control_listen(fd) == -1)
Index: control.c
===
RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
retrieving revision 1.90
diff -u -p -r1.90 control.c
--- control.c 11 Aug 2017 16:02:53 - 1.90
+++ control.c 10 Nov 2018 20:52:46 -
@@ -38,6 +38,32 @@ void control_result(struct ctl_conn *,
ssize_t imsg_read_nofd(struct imsgbuf *);
int
+control_check(char *path)
+{
+ struct sockaddr_un sun;
+ int fd;
+
+ bzero(&sun, sizeof(sun));
+ sun.sun_family = AF_UNIX;
+ strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
+
+ if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) {
+ log_warn("control_check: socket check");
+ return (-1);
+ }
+
+ if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
+ log_warnx("control_check: socket in use");
+ close(fd);
+ return (-1);
+ }
+
+ close(fd);
+
+ return (0);
+}
+
+int
control_init(int restricted, char *path)
{
struct sockaddr_un sun;
Index: session.h
===
RCS file: /cvs/src/usr.sbin/bgpd/session.h,v
retrieving revision 1.125
diff -u -p -r1.125 session.h
--- session.h 24 Oct 2018 08:26:37 - 1.125
+++ session.h 10 Nov 2018 21:53:04 -
@@ -250,6 +250,7 @@ void prepare_listeners(struct bgpd_conf
int get_mpe_label(struct rdomain *);
/* control.c */
+intcontrol_check(char *);
intcontrol_init(int, char *);
intcontrol_listen(int);
void control_shutdown(int);
uticom(4): last driver setting config
Here's another diff to get rid of a driver attaching to a device in
unconfigured state. I'd like to convert them all to enforce that a
USB device can only be matched if the stack already managed to set
its configuration.
I'm looking for tests and oks.
Index: uticom.c
===
RCS file: /cvs/src/sys/dev/usb/uticom.c,v
retrieving revision 1.33
diff -u -p -r1.33 uticom.c
--- uticom.c15 Mar 2018 00:42:41 - 1.33
+++ uticom.c18 Nov 2018 17:11:16 -
@@ -190,7 +190,7 @@ uticom_match(struct device *parent, void
{
struct usb_attach_arg *uaa = aux;
- if (uaa->iface != NULL)
+ if (uaa->iface == NULL || uaa->configno != 1)
return (UMATCH_NONE);
return (usb_lookup(uticom_devs, uaa->vendor, uaa->product) != NULL ?
@@ -240,29 +240,12 @@ uticom_attach_hook(struct device *self)
DPRINTF(("%s: uticom_attach: starting loading firmware\n",
sc->sc_dev.dv_xname));
- err = usbd_set_config_index(sc->sc_udev, UTICOM_CONFIG_INDEX, 1);
- if (err) {
- printf("%s: failed to set configuration: %s\n",
- sc->sc_dev.dv_xname, usbd_errstr(err));
- usbd_deactivate(sc->sc_udev);
- return;
- }
-
/* Get the config descriptor. */
cdesc = usbd_get_config_descriptor(sc->sc_udev);
if (cdesc == NULL) {
printf("%s: failed to get configuration descriptor\n",
sc->sc_dev.dv_xname);
- usbd_deactivate(sc->sc_udev);
- return;
- }
-
- err = usbd_device2interface_handle(sc->sc_udev, UTICOM_IFACE_INDEX,
- &sc->sc_iface);
- if (err) {
- printf("%s: failed to get interface: %s\n",
- sc->sc_dev.dv_xname, usbd_errstr(err));
usbd_deactivate(sc->sc_udev);
return;
}
ubsa(4): don't set the config in match()
Instead of setting the configuration in the *match() routine let the
stack do it for us. This operation can fail, so it's better to not
attach any driver if something bad happens.
This change is similar to what I did with the rest of the drivers some
years ago when I fuzzed the stack.
I'm looking for tests and oks.
Index: ubsa.c
===
RCS file: /cvs/src/sys/dev/usb/ubsa.c,v
retrieving revision 1.66
diff -u -p -r1.66 ubsa.c
--- ubsa.c 27 Apr 2018 08:08:06 - 1.66
+++ ubsa.c 18 Nov 2018 17:01:50 -
@@ -86,9 +86,6 @@ int ubsadebug = 0;
#defineUBSA_MODVER 1 /* module version */
-#defineUBSA_CONFIG_INDEX 1
-#defineUBSA_IFACE_INDEX0
-
#defineUBSA_INTR_INTERVAL 100 /* ms */
#defineUBSA_SET_BAUDRATE 0x00
@@ -225,7 +222,7 @@ ubsa_match(struct device *parent, void *
{
struct usb_attach_arg *uaa = aux;
- if (uaa->iface != NULL)
+ if (uaa->iface == NULL || uaa->configno != 1)
return (UMATCH_NONE);
return (usb_lookup(ubsa_devs, uaa->vendor, uaa->product) != NULL ?
@@ -237,16 +234,15 @@ ubsa_attach(struct device *parent, struc
{
struct ubsa_softc *sc = (struct ubsa_softc *)self;
struct usb_attach_arg *uaa = aux;
- struct usbd_device *dev = uaa->device;
usb_config_descriptor_t *cdesc;
usb_interface_descriptor_t *id;
usb_endpoint_descriptor_t *ed;
const char *devname = sc->sc_dev.dv_xname;
- usbd_status err;
struct ucom_attach_args uca;
int i;
- sc->sc_udev = dev;
+ sc->sc_udev = uaa->device;
+ sc->sc_iface = uaa->iface;
/*
* initialize rts, dtr variables to something
@@ -262,15 +258,6 @@ ubsa_attach(struct device *parent, struc
sc->sc_intr_number = -1;
sc->sc_intr_pipe = NULL;
- /* Move the device into the configured state. */
- err = usbd_set_config_index(dev, UBSA_CONFIG_INDEX, 1);
- if (err) {
- printf("%s: failed to set configuration: %s\n",
- devname, usbd_errstr(err));
- usbd_deactivate(sc->sc_udev);
- goto error;
- }
-
/* get the config descriptor */
cdesc = usbd_get_config_descriptor(sc->sc_udev);
@@ -281,16 +268,6 @@ ubsa_attach(struct device *parent, struc
goto error;
}
- /* get the first interface */
- err = usbd_device2interface_handle(dev, UBSA_IFACE_INDEX,
- &sc->sc_iface);
- if (err) {
- printf("%s: failed to get interface: %s\n",
- devname, usbd_errstr(err));
- usbd_deactivate(sc->sc_udev);
- goto error;
- }
-
/* Find the endpoints */
id = usbd_get_interface_descriptor(sc->sc_iface);
@@ -342,7 +319,7 @@ ubsa_attach(struct device *parent, struc
/* bulkin, bulkout set above */
uca.ibufsizepad = uca.ibufsize;
uca.opkthdrlen = 0;
- uca.device = dev;
+ uca.device = sc->sc_udev;
uca.iface = sc->sc_iface;
uca.methods = &ubsa_methods;
uca.arg = sc;
kue(4): don't set the config in match()
Instead of setting the configuration in the *match() routine let the
stack do it for us. This operation can fail, so it's better to not
attach any driver if something bad happens.
This change is similar to what I did with the rest of the drivers some
years ago when I fuzzed the stack.
I'm looking for tests and oks.
Index: if_kue.c
===
RCS file: /cvs/src/sys/dev/usb/if_kue.c,v
retrieving revision 1.89
diff -u -p -r1.89 if_kue.c
--- if_kue.c2 Oct 2018 19:49:10 - 1.89
+++ if_kue.c18 Nov 2018 16:53:02 -
@@ -389,9 +389,7 @@ kue_match(struct device *parent, void *m
{
struct usb_attach_arg *uaa = aux;
- DPRINTFN(25,("kue_match: enter\n"));
-
- if (uaa->iface != NULL)
+ if (uaa->iface == NULL || uaa->configno != KUE_CONFIG_NO)
return (UMATCH_NONE);
return (usb_lookup(kue_devs, uaa->vendor, uaa->product) != NULL ?
@@ -405,7 +403,7 @@ kue_attachhook(struct device *self)
int s;
struct ifnet*ifp;
struct usbd_device *dev = sc->kue_udev;
- struct usbd_interface *iface;
+ struct usbd_interface *iface = sc->kue_iface;
usbd_status err;
usb_interface_descriptor_t *id;
usb_endpoint_descriptor_t *ed;
@@ -418,14 +416,6 @@ kue_attachhook(struct device *self)
return;
}
- err = usbd_device2interface_handle(dev, KUE_IFACE_IDX, &iface);
- if (err) {
- printf("%s: getting interface handle failed\n",
- sc->kue_dev.dv_xname);
- return;
- }
-
- sc->kue_iface = iface;
id = usbd_get_interface_descriptor(iface);
/* Find endpoints. */
@@ -510,20 +500,13 @@ kue_attach(struct device *parent, struct
struct kue_softc*sc = (struct kue_softc *)self;
struct usb_attach_arg *uaa = aux;
struct usbd_device *dev = uaa->device;
- usbd_status err;
DPRINTFN(5,(" : kue_attach: sc=%p, dev=%p", sc, dev));
- err = usbd_set_config_no(dev, KUE_CONFIG_NO, 1);
- if (err) {
- printf("%s: setting config no failed\n",
- sc->kue_dev.dv_xname);
- return;
- }
-
sc->kue_udev = dev;
sc->kue_product = uaa->product;
sc->kue_vendor = uaa->vendor;
+ sc->kue_iface = uaa->iface;
config_mountroot(self, kue_attachhook);
}
makemap.8 patch
Use new syntax. Index: makemap.8 === RCS file: /cvs/src/usr.sbin/smtpd/makemap.8,v retrieving revision 1.29 diff -u -p -u -r1.29 makemap.8 --- makemap.8 13 Feb 2016 08:53:18 - 1.29 +++ makemap.8 18 Nov 2018 14:29:33 - @@ -105,8 +105,11 @@ In addition to adding an entry to the pr one must add a filter rule that accepts mail for the domain map, for example: .Bd -literal -offset indent -table domains "/etc/mail/domains" -accept for domain deliver to mbox +table domains db:/etc/mail/domains.db + +action "local" mbox + +match for domain action "local" .Ed .Sh VIRTUAL DOMAINS Virtual domains may also be kept in tables. @@ -140,11 +143,13 @@ In addition to adding an entry to the vi one must add a filter rule that accepts mail for virtual domains, for example: .Bd -literal -offset indent -table vdomains "/etc/mail/vdomains" -table vusers "/etc/mail/users" +table vdomains db:/etc/mail/vdomains.db +table vusers db:/etc/mail/users.db + +action "local" mbox virtual -accept for domain virtual deliver to mbox -accept for domain example.org virtual deliver to mbox +match for domain action "local" +match for domain "example.org" action "local" .Ed .Sh FILES .Bl -tag -width "/etc/mail/aliasesXXX" -compact
Re: 6.4 openBGPD Segfault caused by filters referencing undeclared prefix-set
Hello Stuart Thanks for the helpful advice on giving a better crash report i will do that going forward... On Sun 18 Nov 2018, 10:50 Stuart Henderson On 2018/11/18 08:58, Tom Smyth wrote: > > I have attached the coredump > > Generally the coredump by itself isn't that useful to others as it > needs the right binary to go with it, a backtrace is usually better: > > gdb /usr/sbin/bgpd /path/to/bgpd.core > bt > > For reference, in some cases multiple processes will crash, because > the filenames are all "bgpd.core" they can overwrite each other. > See the bottom of the sysctl manpage for a way around that. > >
Re: 6.4 openBGPD Segfault caused by filters referencing undeclared prefix-set
On 2018/11/18 08:58, Tom Smyth wrote: > I have attached the coredump Generally the coredump by itself isn't that useful to others as it needs the right binary to go with it, a backtrace is usually better: gdb /usr/sbin/bgpd /path/to/bgpd.core bt For reference, in some cases multiple processes will crash, because the filenames are all "bgpd.core" they can overwrite each other. See the bottom of the sysctl manpage for a way around that.
Re: bktr(4): remove useless assignment
On Sat, Nov 17, 2018 at 04:36:49PM -0700, Theo de Raadt wrote: > Why not delete the 2nd line? > > I mean isn't it obvious? I'd expect some archeology to figure > out how this bug got introduced, rather than a diff which you > obviously don't know the result of. Fair enough, I realize it indeed looks that way if I don't mention anything. Before sending the diff I found out that both assignements are in our tree since the driver got imported from FreeBSD in 2001 and remained untouched since r1.1. I then checked the actual state of yuvpack_prog() in FreeBSD and only the second assignment remains. The second assignment in its current form got introduced in FreeBSD in 1997 with r29233 [1], when syncinc the driver. The first assignment got removed in 2017 with r314147 [2]. [1]: https://svnweb.freebsd.org/base?view=revision&revision=29233 [2]: https://svnweb.freebsd.org/base?view=revision&revision=314147
Re: 6.4 openBGPD Segfault caused by filters referencing undeclared prefix-set
On Sun, Nov 18, 2018 at 09:01:06AM +, Tom Smyth wrote:
> just to confim
> im running 6.4 GENERIC.MP#364 amd64
> On Sun, 18 Nov 2018 at 08:58, Tom Smyth wrote:
> >
> > Hello,
> > I was configuring an openbsd 6.4 bgpd router using the
> > /etc/examples/bgpd.conf as a template
> >
> > If you comment out the prefix-set mynetworks
> > # list of networks that may be originated by our ASN
> > #prefix-set mynetworks {\
> > #192.0.2.0/24\
> > #2001:db8:abcd::/48\
> > #}
> >
> > and leave filters that depend on the prefix-set
> > in the bgpd.conf file
> >
> > bgpd segfaults on startup as shown below
> >
> > corertfw2# bgpd -dv
> > startup
> > rereading config
> > session engine ready
> > ASN = "62129"
> > peer closed imsg connection
> > Segmentation fault (core dumped)
> > SE: Lost connection to parent
> > session engine exiting
> > corertfw2# route decision engine ready
> > peer closed imsg connection
> > fatal in RDE: Lost connection to parent
> >
> >
> > I have attached the coredump
> >
> > Removing filters that reference undeclared prefix-set mynetworks
> > resolved the issue ...
> >
> > --
> > Kindest regards,
> > Tom Smyth
>
Thanks for the report. Fixed in parse.y rev 1.363.
The problem is not the filter referencing the non existing prefix-set. It
is the network statement a bit higher up.
--
:wq Claudio
Re: 6.4 openBGPD Segfault caused by filters referencing undeclared prefix-set
On Sun, Nov 18, 2018 at 10:30:32AM +0100, Florian Obser wrote:
> diff --git parse.y parse.y
> index 7b7ce5388c0..1aa2aabc28c 100644
> --- parse.y
> +++ parse.y
> @@ -879,7 +879,7 @@ network : NETWORK prefix filter_set {
> struct network *n;
> if ((ps = find_prefixset($3, &conf->prefixsets))
> == NULL) {
> - yyerror("prefix-set %s not defined", ps->name);
> + yyerror("prefix-set %s not defined", $3);
> free($3);
> filterset_free($4);
> free($4);
>
> OK?
Just committed the same since it was obvious.
> On Sun, Nov 18, 2018 at 08:58:33AM +, Tom Smyth wrote:
> > Hello,
> > I was configuring an openbsd 6.4 bgpd router using the
> > /etc/examples/bgpd.conf as a template
> >
> > If you comment out the prefix-set mynetworks
> > # list of networks that may be originated by our ASN
> > #prefix-set mynetworks {\
> > #192.0.2.0/24\
> > #2001:db8:abcd::/48\
> > #}
> >
> > and leave filters that depend on the prefix-set
> > in the bgpd.conf file
> >
> > bgpd segfaults on startup as shown below
> >
> > corertfw2# bgpd -dv
> > startup
> > rereading config
> > session engine ready
> > ASN = "62129"
> > peer closed imsg connection
> > Segmentation fault (core dumped)
> > SE: Lost connection to parent
> > session engine exiting
> > corertfw2# route decision engine ready
> > peer closed imsg connection
> > fatal in RDE: Lost connection to parent
> >
> >
> > I have attached the coredump
> >
> > Removing filters that reference undeclared prefix-set mynetworks
> > resolved the issue ...
> >
> > --
> > Kindest regards,
> > Tom Smyth
>
>
>
> --
> I'm not entirely sure you are real.
>
--
:wq Claudio
Re: 6.4 openBGPD Segfault caused by filters referencing undeclared prefix-set
diff --git parse.y parse.y
index 7b7ce5388c0..1aa2aabc28c 100644
--- parse.y
+++ parse.y
@@ -879,7 +879,7 @@ network : NETWORK prefix filter_set {
struct network *n;
if ((ps = find_prefixset($3, &conf->prefixsets))
== NULL) {
- yyerror("prefix-set %s not defined", ps->name);
+ yyerror("prefix-set %s not defined", $3);
free($3);
filterset_free($4);
free($4);
OK?
On Sun, Nov 18, 2018 at 08:58:33AM +, Tom Smyth wrote:
> Hello,
> I was configuring an openbsd 6.4 bgpd router using the
> /etc/examples/bgpd.conf as a template
>
> If you comment out the prefix-set mynetworks
> # list of networks that may be originated by our ASN
> #prefix-set mynetworks {\
> #192.0.2.0/24\
> #2001:db8:abcd::/48\
> #}
>
> and leave filters that depend on the prefix-set
> in the bgpd.conf file
>
> bgpd segfaults on startup as shown below
>
> corertfw2# bgpd -dv
> startup
> rereading config
> session engine ready
> ASN = "62129"
> peer closed imsg connection
> Segmentation fault (core dumped)
> SE: Lost connection to parent
> session engine exiting
> corertfw2# route decision engine ready
> peer closed imsg connection
> fatal in RDE: Lost connection to parent
>
>
> I have attached the coredump
>
> Removing filters that reference undeclared prefix-set mynetworks
> resolved the issue ...
>
> --
> Kindest regards,
> Tom Smyth
--
I'm not entirely sure you are real.
Re: 6.4 openBGPD Segfault caused by filters referencing undeclared prefix-set
just to confim
im running 6.4 GENERIC.MP#364 amd64
On Sun, 18 Nov 2018 at 08:58, Tom Smyth wrote:
>
> Hello,
> I was configuring an openbsd 6.4 bgpd router using the
> /etc/examples/bgpd.conf as a template
>
> If you comment out the prefix-set mynetworks
> # list of networks that may be originated by our ASN
> #prefix-set mynetworks {\
> #192.0.2.0/24\
> #2001:db8:abcd::/48\
> #}
>
> and leave filters that depend on the prefix-set
> in the bgpd.conf file
>
> bgpd segfaults on startup as shown below
>
> corertfw2# bgpd -dv
> startup
> rereading config
> session engine ready
> ASN = "62129"
> peer closed imsg connection
> Segmentation fault (core dumped)
> SE: Lost connection to parent
> session engine exiting
> corertfw2# route decision engine ready
> peer closed imsg connection
> fatal in RDE: Lost connection to parent
>
>
> I have attached the coredump
>
> Removing filters that reference undeclared prefix-set mynetworks
> resolved the issue ...
>
> --
> Kindest regards,
> Tom Smyth
