Re: Add PCI ID for VirtualBox NVMe

2018-10-25 Thread Mike Larkin
On Thu, Jul 19, 2018 at 07:19:55PM -0500, Andrew Daugherity wrote:
> This is strictly a cosmetic change, as it already gets bound to
> nvme(4) and works properly, but is currently identified as an unknown
> product.
> 
> 
> diff --git a/sys/dev/pci/pcidevs b/sys/dev/pci/pcidevs
> index f9ab4d1c535..e50b8d1a1a7 100644
> --- a/sys/dev/pci/pcidevs
> +++ b/sys/dev/pci/pcidevs
> @@ -2912,6 +2912,7 @@ product INITIO INIC941 0x9401 INIC-941
>  product INITIO INIC950 0x9500 INIC-950
> 
>  /* InnoTek Systemberatung GmbH */
> +product INNOTEK VBNVME 0x4e56 VirtualBox NVMe
>  product INNOTEK VBGA 0xbeef VirtualBox Graphics Adapter
>  product INNOTEK VBGS 0xcafe VirtualBox Guest Service
> 
> 
> 
> Change in dmesg:
> -nvme0 at pci0 dev 14 function 0 vendor "InnoTek", unknown product
> 0x4e56 rev 0x00: apic 2 int 22, NVMe 1.2
> +nvme0 at pci0 dev 14 function 0 "InnoTek VirtualBox NVMe" rev 0x00:
> apic 2 int 22, NVMe 1.2
>  nvme0: ORCL-VBOX-NVME-VER12, firmware 1.0, serial VB1234-56789
>  scsibus1 at nvme0: 1 targets
>  sd0 at scsibus1 targ 0 lun 0:  SCSI4
> 0/direct fixed
> 
> 
> -Andrew
> 

Sorry this took so long, I was cleaning out my inbox and saw this. Committed,
thanks!

-ml



make nc(4) print what went wrong with unix domain sockets

2018-10-25 Thread David Gwynne
nc looks like it just does nothing without it. it's nice to see things
like "No such file" or "Permission denied".

ok?

Index: netcat.c
===
RCS file: /cvs/src/usr.bin/nc/netcat.c,v
retrieving revision 1.191
diff -u -p -r1.191 netcat.c
--- netcat.c27 Apr 2018 15:17:53 -  1.191
+++ netcat.c26 Oct 2018 04:57:19 -
@@ -616,8 +616,10 @@ main(int argc, char *argv[])
if (!zflag)
readwrite(s, NULL);
close(s);
-   } else
+   } else {
+   warn("%s", host);
ret = 1;
+   }
 
if (uflag)
unlink(unix_dg_tmp_socket);
@@ -855,8 +857,8 @@ unix_connect(char *path)
errno = save_errno;
return -1;
}
-   return s;
 
+   return s;
 }
 
 /*



Re: Reuse VM ids.

2018-10-25 Thread Reyk Floeter
On Tue, Oct 23, 2018 at 10:21:08PM -0700, Ori Bernstein wrote:
> On Mon, 8 Oct 2018 07:59:15 -0700, Bob Beck  wrote:
> 
> > works here and I like it.  but probably for after unlock
> > 
> 
> It's after unlock -- pinging for OKs.
> 

Not yet.  Please include the VM's uid in the claim, e.g.

claim_vmid(const char *name, uid_t uid)

It is not a strong protection, but it doesn't make sense that other
users can run a VM with the same name and get the claimed Id.

Reyk

> diff --git usr.sbin/vmd/config.c usr.sbin/vmd/config.c
> index af12b790002..522bae32501 100644
> --- usr.sbin/vmd/config.c
> +++ usr.sbin/vmd/config.c
> @@ -61,7 +61,10 @@ config_init(struct vmd *env)
>   if (what & CONFIG_VMS) {
>   if ((env->vmd_vms = calloc(1, sizeof(*env->vmd_vms))) == NULL)
>   return (-1);
> + if ((env->vmd_known = calloc(1, sizeof(*env->vmd_known))) == 
> NULL)
> + return (-1);
>   TAILQ_INIT(env->vmd_vms);
> + TAILQ_INIT(env->vmd_known);
>   }
>   if (what & CONFIG_SWITCHES) {
>   if ((env->vmd_switches = calloc(1,
> diff --git usr.sbin/vmd/vmd.c usr.sbin/vmd/vmd.c
> index 18a5e0d3d5d..732691b4381 100644
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -1169,6 +1169,27 @@ vm_remove(struct vmd_vm *vm, const char *caller)
>   free(vm);
>  }
>  
> +static uint32_t
> +claim_vmid(const char *name)
> +{
> + struct name2id *n2i = NULL;
> +
> + TAILQ_FOREACH(n2i, env->vmd_known, entry)
> + if (strcmp(n2i->name, name) == 0)
> + return n2i->id;
> +
> + if (++env->vmd_nvm == 0)
> + fatalx("too many vms");
> + if ((n2i = calloc(1, sizeof(struct name2id))) == NULL)
> + fatalx("could not alloc vm name");
> + n2i->id = env->vmd_nvm;
> + if (strlcpy(n2i->name, name, sizeof(n2i->name)) >= sizeof(n2i->name))
> + fatalx("overlong vm name");
> + TAILQ_INSERT_TAIL(env->vmd_known, n2i, entry);
> +
> + return n2i->id;
> +}
> +
>  int
>  vm_register(struct privsep *ps, struct vmop_create_params *vmc,
>  struct vmd_vm **ret_vm, uint32_t id, uid_t uid)
> @@ -1300,11 +1321,8 @@ vm_register(struct privsep *ps, struct 
> vmop_create_params *vmc,
>   vm->vm_cdrom = -1;
>   vm->vm_iev.ibuf.fd = -1;
>  
> - if (++env->vmd_nvm == 0)
> - fatalx("too many vms");
> -
>   /* Assign a new internal Id if not specified */
> - vm->vm_vmid = id == 0 ? env->vmd_nvm : id;
> + vm->vm_vmid = (id == 0) ? claim_vmid(vcp->vcp_name) : id;
>  
>   log_debug("%s: registering vm %d", __func__, vm->vm_vmid);
>   TAILQ_INSERT_TAIL(env->vmd_vms, vm, vm_entry);
> diff --git usr.sbin/vmd/vmd.h usr.sbin/vmd/vmd.h
> index b7c012854e8..86fad536e59 100644
> --- usr.sbin/vmd/vmd.h
> +++ usr.sbin/vmd/vmd.h
> @@ -276,6 +276,13 @@ struct vmd_user {
>  };
>  TAILQ_HEAD(userlist, vmd_user);
>  
> +struct name2id {
> + charname[VMM_MAX_NAME_LEN];
> + int32_t id;
> + TAILQ_ENTRY(name2id)entry;
> +};
> +TAILQ_HEAD(name2idlist, name2id);
> +
>  struct address {
>   struct sockaddr_storage  ss;
>   int  prefixlen;
> @@ -300,6 +307,7 @@ struct vmd {
>  
>   uint32_t vmd_nvm;
>   struct vmlist   *vmd_vms;
> + struct name2idlist  *vmd_known;
>   uint32_t vmd_nswitches;
>   struct switchlist   *vmd_switches;
>   struct userlist *vmd_users;
> 



Add new PCI product IDs

2018-10-25 Thread Peter Ezetta
Hello,

Diff below adds product IDs for the Nvidia Quadro M1200 Mobile graphics
card and the Intel Xeon E3-1200 v6 7th gen Host Bridge (for mobile).

Index: pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1863
diff -u -p -r1.1863 pcidevs
--- pcidevs 22 Oct 2018 05:06:32 -  1.1863
+++ pcidevs 25 Oct 2018 21:36:12 -
@@ -4718,6 +4718,7 @@ product INTEL CORE7G_U_HB 0x5904  Core 7G
 product INTEL CORE7G_U_GT1 0x5906  HD Graphics 610
 product INTEL CORE7G_Y_HB  0x590c  Core 7G Host
 product INTEL CORE7G_Y_GT1 0x590e  HD Graphics
+product INTEL XEONE3_1200V6M_HB0x5910  Xeon E3-1200 v6/7 Host
 product INTEL CORE_GMM_2   0x5911  Core GMM
 product INTEL CORE7G_S_GT2 0x5912  HD Graphics 630
 product INTEL CORE8G_U_HB  0x5914  Core 8G Host
@@ -6529,6 +6530,7 @@ product NVIDIA GEFORCE940MX   0x134d  GeFor
 product NVIDIA GEFORCEGTX750TI 0x1380  GeForce GTX 750 Ti
 product NVIDIA GEFORCEGTX750   0x1381  GeForce GTX 750
 product NVIDIA GEFORCEGTX745   0x1382  GeForce GTX 745
+product NVIDIA QUADROM1200 0x13b6  Quadro M1200

 /* Oak Technologies products */
 product OAKTECH OTI10070x0107  OTI107


The X hole

2018-10-25 Thread Theo de Raadt
In his role at X.org in the security team, Matthieu says he became aware
of this bug on the 11th.

He did not tell any of us at OpenBSD.

We were made aware bit more than 1 hour before public information went
out.

We were in the midst of an early OpenBSD release.  If we had known, the
OpenBSD 6.4 release could have been held back a week or two, till today.
It would have been easy.

Or even easier, we could have made a late decision to disable legacy
drivers and lost the setuid bit.  Which will probably happen in a commit
later today.

But we were not made aware, therefore OpenBSD 6.4 is also affected.

As yet we don't have answers about why our X maintainer (on the X
security team) and his team provided information to other projects (some
who don't even ship with this new X server) but chose to not give us a
heads-up which could have saved all the new 6.4 users a lot of grief.

I don't understand how it happened.

>From my point of view we all share a goal of getting fixes and
preventative methods out to the community as fast as possible.  Here an
artificial delay was created which left a trivial vulnerability *known
to the upstream* on everyone's machine, in an operating system with
a *well known and published release cycle*.

I feel an abdication of the duty of care occured here.

That is the first localhost root hole in quite a long time.



Re: bgpd: replace some more walkers with rib_dump

2018-10-25 Thread Claudio Jeker
On Thu, Oct 25, 2018 at 09:04:18PM +0200, Denis Fondras wrote:
> On Thu, Oct 25, 2018 at 08:51:30AM +0200, Claudio Jeker wrote:
> > Next step on my quest to make the RIB code better.
> > This changes the following things:
> > - network_flush is now using rib_dump_new to walk the Adj-RIB-In and
> >   remove all dynamically added announcements
> > - peer_flush got generalized and is now used also in peer_down.
> >   It also uses a rib_dump_new call to walk the Adj-RIB-In and remove
> >   all prefixes from a peer but this is done synchronous for now.
> > - peer_flush is now working correctly for AID_UNSPEC.
> > - Change the rib_valid check to continue instead of break out of the loop.
> >   The rib table can have holes so the loop needs to continue.
> > 
> > This removes the last three places that use the peer -> path -> prefix
> > tailqs so the next step is to remove these and make rde_aspath just an
> > object that is referenced by prefixes. After that adding a proper
> > Adj-RIB-Out should be possible.
> > 
> 
> Question below.
> Running on prod too :)
> 
> > Index: rde_rib.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> > retrieving revision 1.180
> > diff -u -p -r1.180 rde_rib.c
> > --- rde_rib.c   24 Oct 2018 08:26:37 -  1.180
> > +++ rde_rib.c   24 Oct 2018 08:44:54 -
> > @@ -339,8 +339,8 @@ rib_restart(struct rib_context *ctx)
> >  static void
> >  rib_dump_r(struct rib_context *ctx)
> >  {
> > +   struct rib_entry*re, *next;
> > struct rib  *rib;
> > -   struct rib_entry*re;
> > unsigned int i;
> >  
> > rib = rib_byid(ctx->ctx_rib_id);
> > @@ -352,7 +352,8 @@ rib_dump_r(struct rib_context *ctx)
> > else
> > re = rib_restart(ctx);
> >  
> > -   for (i = 0; re != NULL; re = RB_NEXT(rib_tree, unused, re)) {
> > +   for (i = 0; re != NULL; re = next) {
> > +   next = RB_NEXT(rib_tree, unused, re);
> 
> Why not RB_FOREACH_SAFE ?

Because we do not start with RB_MIN. The inital element is set above and
so we can't use a regular RB_FOREACH loop either.
 
> > if (re->rib_id != ctx->ctx_rib_id)
> > fatalx("%s: Unexpected RIB %u != %u.", __func__,
> > re->rib_id, ctx->ctx_rib_id);
> > @@ -435,6 +436,10 @@ rib_dump_new(u_int16_t id, u_int8_t aid,
> >  
> > LIST_INSERT_HEAD(_dumps, ctx, entry);
> >  
> > +   /* requested a sync traversal */
> > +   if (count == 0)
> > +   rib_dump_r(ctx);
> > +
> > return 0;
> >  }
> >  
> > @@ -664,65 +669,6 @@ path_lookup(struct rde_aspath *aspath, s
> > return (NULL);
> >  }
> >  
> > -void
> > -path_remove(struct rde_aspath *asp)
> > -{
> > -   struct prefix   *p, *np;
> > -
> > -   for (p = TAILQ_FIRST(>prefixes); p != NULL; p = np) {
> > -   np = TAILQ_NEXT(p, path_l);
> > -   if (asp->pftableid) {
> > -   struct bgpd_addr addr;
> > -
> > -   pt_getaddr(p->re->prefix, );
> > -   /* Commit is done in peer_down() */
> > -   rde_send_pftable(prefix_aspath(p)->pftableid, ,
> > -   p->re->prefix->prefixlen, 1);
> > -   }
> > -   prefix_destroy(p);
> > -   }
> > -}
> > -
> > -/* remove all stale routes or if staletime is 0 remove all routes for
> > -   a specified AID. */
> > -u_int32_t
> > -path_remove_stale(struct rde_aspath *asp, u_int8_t aid, time_t staletime)
> > -{
> > -   struct prefix   *p, *np;
> > -   u_int32_trprefixes;
> > -
> > -   rprefixes=0;
> > -   /*
> > -* This is called when a session flapped and during that time
> > -* the pending updates for that peer are getting reset.
> > -*/
> > -   for (p = TAILQ_FIRST(>prefixes); p != NULL; p = np) {
> > -   np = TAILQ_NEXT(p, path_l);
> > -   if (p->re->prefix->aid != aid)
> > -   continue;
> > -
> > -   if (staletime && p->lastchange > staletime)
> > -   continue;
> > -
> > -   if (asp->pftableid) {
> > -   struct bgpd_addr addr;
> > -
> > -   pt_getaddr(p->re->prefix, );
> > -   /* Commit is done in peer_flush() */
> > -   rde_send_pftable(prefix_aspath(p)->pftableid, ,
> > -   p->re->prefix->prefixlen, 1);
> > -   }
> > -
> > -   /* only count Adj-RIB-In */
> > -   if (re_rib(p->re) == [RIB_ADJ_IN].rib)
> > -   rprefixes++;
> > -
> > -   prefix_destroy(p);
> > -   }
> > -   return (rprefixes);
> > -}
> > -
> > -
> >  /*
> >   * This function can only called when all prefix have been removed first.
> >   * Normally this happens directly out of the prefix removal functions.
> > @@ -1144,29 +1090,6 @@ prefix_destroy(struct prefix *p)
> >  
> > if (path_empty(asp))
> > path_destroy(asp);
> > -}
> > -
> > -/*
> > - * helper function 

Re: bgpd: replace some more walkers with rib_dump

2018-10-25 Thread Denis Fondras
On Thu, Oct 25, 2018 at 08:51:30AM +0200, Claudio Jeker wrote:
> Next step on my quest to make the RIB code better.
> This changes the following things:
> - network_flush is now using rib_dump_new to walk the Adj-RIB-In and
>   remove all dynamically added announcements
> - peer_flush got generalized and is now used also in peer_down.
>   It also uses a rib_dump_new call to walk the Adj-RIB-In and remove
>   all prefixes from a peer but this is done synchronous for now.
> - peer_flush is now working correctly for AID_UNSPEC.
> - Change the rib_valid check to continue instead of break out of the loop.
>   The rib table can have holes so the loop needs to continue.
> 
> This removes the last three places that use the peer -> path -> prefix
> tailqs so the next step is to remove these and make rde_aspath just an
> object that is referenced by prefixes. After that adding a proper
> Adj-RIB-Out should be possible.
> 

Question below.
Running on prod too :)

> Index: rde_rib.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 rde_rib.c
> --- rde_rib.c 24 Oct 2018 08:26:37 -  1.180
> +++ rde_rib.c 24 Oct 2018 08:44:54 -
> @@ -339,8 +339,8 @@ rib_restart(struct rib_context *ctx)
>  static void
>  rib_dump_r(struct rib_context *ctx)
>  {
> + struct rib_entry*re, *next;
>   struct rib  *rib;
> - struct rib_entry*re;
>   unsigned int i;
>  
>   rib = rib_byid(ctx->ctx_rib_id);
> @@ -352,7 +352,8 @@ rib_dump_r(struct rib_context *ctx)
>   else
>   re = rib_restart(ctx);
>  
> - for (i = 0; re != NULL; re = RB_NEXT(rib_tree, unused, re)) {
> + for (i = 0; re != NULL; re = next) {
> + next = RB_NEXT(rib_tree, unused, re);

Why not RB_FOREACH_SAFE ?

>   if (re->rib_id != ctx->ctx_rib_id)
>   fatalx("%s: Unexpected RIB %u != %u.", __func__,
>   re->rib_id, ctx->ctx_rib_id);
> @@ -435,6 +436,10 @@ rib_dump_new(u_int16_t id, u_int8_t aid,
>  
>   LIST_INSERT_HEAD(_dumps, ctx, entry);
>  
> + /* requested a sync traversal */
> + if (count == 0)
> + rib_dump_r(ctx);
> +
>   return 0;
>  }
>  
> @@ -664,65 +669,6 @@ path_lookup(struct rde_aspath *aspath, s
>   return (NULL);
>  }
>  
> -void
> -path_remove(struct rde_aspath *asp)
> -{
> - struct prefix   *p, *np;
> -
> - for (p = TAILQ_FIRST(>prefixes); p != NULL; p = np) {
> - np = TAILQ_NEXT(p, path_l);
> - if (asp->pftableid) {
> - struct bgpd_addr addr;
> -
> - pt_getaddr(p->re->prefix, );
> - /* Commit is done in peer_down() */
> - rde_send_pftable(prefix_aspath(p)->pftableid, ,
> - p->re->prefix->prefixlen, 1);
> - }
> - prefix_destroy(p);
> - }
> -}
> -
> -/* remove all stale routes or if staletime is 0 remove all routes for
> -   a specified AID. */
> -u_int32_t
> -path_remove_stale(struct rde_aspath *asp, u_int8_t aid, time_t staletime)
> -{
> - struct prefix   *p, *np;
> - u_int32_trprefixes;
> -
> - rprefixes=0;
> - /*
> -  * This is called when a session flapped and during that time
> -  * the pending updates for that peer are getting reset.
> -  */
> - for (p = TAILQ_FIRST(>prefixes); p != NULL; p = np) {
> - np = TAILQ_NEXT(p, path_l);
> - if (p->re->prefix->aid != aid)
> - continue;
> -
> - if (staletime && p->lastchange > staletime)
> - continue;
> -
> - if (asp->pftableid) {
> - struct bgpd_addr addr;
> -
> - pt_getaddr(p->re->prefix, );
> - /* Commit is done in peer_flush() */
> - rde_send_pftable(prefix_aspath(p)->pftableid, ,
> - p->re->prefix->prefixlen, 1);
> - }
> -
> - /* only count Adj-RIB-In */
> - if (re_rib(p->re) == [RIB_ADJ_IN].rib)
> - rprefixes++;
> -
> - prefix_destroy(p);
> - }
> - return (rprefixes);
> -}
> -
> -
>  /*
>   * This function can only called when all prefix have been removed first.
>   * Normally this happens directly out of the prefix removal functions.
> @@ -1144,29 +1090,6 @@ prefix_destroy(struct prefix *p)
>  
>   if (path_empty(asp))
>   path_destroy(asp);
> -}
> -
> -/*
> - * helper function to clean up the dynamically added networks
> - */
> -void
> -prefix_network_clean(struct rde_peer *peer)
> -{
> - struct rde_aspath   *asp, *xasp;
> - struct prefix   *p, *xp;
> -
> - for (asp = TAILQ_FIRST(>path_h); asp != NULL; asp = xasp) {
> - xasp = TAILQ_NEXT(asp, peer_l);
> - if ((asp->flags & 

bgplg: fix crash (error 500)

2018-10-25 Thread Denis Fondras
Just make sure arg is not NULL in lg_getarg() before accessing. It happens when
input contains an invalid character.

Unrelated but while at it, abort earlier in lg_arg2argv() if there is no
argument available.

Index: bgplg.c
===
RCS file: /cvs/src/usr.bin/bgplg/bgplg.c,v
retrieving revision 1.19
diff -u -p -r1.19 bgplg.c
--- bgplg.c 5 Mar 2018 10:53:37 -   1.19
+++ bgplg.c 25 Oct 2018 18:18:55 -
@@ -134,6 +134,9 @@ lg_getarg(const char *name, char *arg, i
size_t namelen, ptrlen;
int i;
 
+   if (arg == NULL)
+   return (NULL);
+
namelen = strlen(name);
 
for (i = 0; i < len; i++) {
@@ -171,6 +174,9 @@ lg_arg2argv(char *arg, int *argc)
c++;
}
}
+
+   if (arg[0] == '\0')
+   return (NULL);
 
/* Generate array */
if ((argv = calloc(c + 1, sizeof(char *))) == NULL) {



Re: bgplg: allow neighbors with space in name

2018-10-25 Thread Denis Fondras
On Thu, Oct 25, 2018 at 02:04:10PM +0100, Stuart Henderson wrote:
> On 2018/10/24 17:38, Denis Fondras wrote:
> > I have peers with description containing spaces but bgplg won't accept that 
> > by
> > default.
> > 
> > I'd like some comments on that diff.
> > It is OK for bgplgsh (show ip bgp in "Peer 1" feels OK) but not for bgplg 
> > as I
> > have to quote the peer description in the input box (feels rather 
> > unnatural).
> 
> it feels like allowing " in allowed characters for the CGI is starting
> to open a can of worms..
> 
> personally I think I'd change the descriptions to avoid spaces.
> 

Thanks to everyone who respond.
Dropping the idea and changing my peer description :)



Re: /bin/df align inode output

2018-10-25 Thread Todd C. Miller
With your patch the mount point is offset by a space from the
"Mounted on" header.

 - todd



Re: unveil getconf

2018-10-25 Thread Todd C. Miller
On Thu, 25 Oct 2018 11:19:34 +0100, Ricardo Mestre wrote:

> The code path were we pass `pathname' in the arguments is already limited
> with pledge(2), but since we know exactly what it is then we can go further
> and also unveil(2) it with read permissions.

OK millert@

 - todd



Re: bgplg: allow neighbors with space in name

2018-10-25 Thread Theo de Raadt
Stuart Henderson  wrote:

> On 2018/10/24 17:38, Denis Fondras wrote:
> > I have peers with description containing spaces but bgplg won't accept that 
> > by
> > default.
> > 
> > I'd like some comments on that diff.
> > It is OK for bgplgsh (show ip bgp in "Peer 1" feels OK) but not for bgplg 
> > as I
> > have to quote the peer description in the input box (feels rather 
> > unnatural).
> 
> it feels like allowing " in allowed characters for the CGI is starting
> to open a can of worms..
> 
> personally I think I'd change the descriptions to avoid spaces.

I also think it is very dangerous, and I don't know why people put
spaces into those names.

Yet I see it all the time.  Maybe our documentation needs to change,
and also the damn examples.

I see people struggling all the time, and I don't see why we fix this
through example

The problem starts in the example:

group "ibgp mesh v4" {
...
descr "IPv4 Transit Provider A"

etc



Re: bgplg: allow neighbors with space in name

2018-10-25 Thread Tom Smyth
Hello Denis, Stuart, all,
I think what Stuart is saying regarding double quotes makes sense

we try to avoid spaces  where possible makes life easier ...

Thanks
Tom Smyth
On Thu, 25 Oct 2018 at 14:06, Stuart Henderson  wrote:
>
> On 2018/10/24 17:38, Denis Fondras wrote:
> > I have peers with description containing spaces but bgplg won't accept that 
> > by
> > default.
> >
> > I'd like some comments on that diff.
> > It is OK for bgplgsh (show ip bgp in "Peer 1" feels OK) but not for bgplg 
> > as I
> > have to quote the peer description in the input box (feels rather 
> > unnatural).
>
> it feels like allowing " in allowed characters for the CGI is starting
> to open a can of worms..
>
> personally I think I'd change the descriptions to avoid spaces.
>


-- 
Kindest regards,
Tom Smyth

Mobile: +353 87 6193172
The information contained in this E-mail is intended only for the
confidential use of the named recipient. If the reader of this message
is not the intended recipient or the person responsible for
delivering it to the recipient, you are hereby notified that you have
received this communication in error and that any review,
dissemination or copying of this communication is strictly prohibited.
If you have received this in error, please notify the sender
immediately by telephone at the number above and erase the message
You are requested to carry out your own virus check before
opening any attachment.



Re: Fix descriptions of smtps vs smtp+tls in smtpd.conf.5

2018-10-25 Thread trondd
On Thu, October 25, 2018 2:24 am, Raf Czlonka wrote:
> On Thu, Oct 25, 2018 at 07:11:47AM BST, Gilles Chehade wrote:
>>
>> smtpd will _always_ display a 'starttls' log line when the TLS channel
>> starts,
>> disregarding if TLS was started at connect time (smtps) or within the
>> protocol
>> (smtp+tls, or even smtp since it does opportunistic tls).
>>
>
> I guess this is the confusing bit - seeing 'starttls' in the log
> file and thinking 'STARTTLS', i.e. the "TLS upgrade".
>
> R.
>

Yes, I mistakenly assumed that where it didn't log "starttls" it wasn't
using STARTTLS and therefore using TLS and where it did log "starttls"
meant it was using STARTTLS.  Silly me. :P

Unfortunatly I also didn't know which my ISP was actually using since
before, with secure:// it would try both and always sent mail.  I should
have figured out what I was dealing with first.



Re: bgplg: allow neighbors with space in name

2018-10-25 Thread Stuart Henderson
On 2018/10/24 17:38, Denis Fondras wrote:
> I have peers with description containing spaces but bgplg won't accept that by
> default.
> 
> I'd like some comments on that diff.
> It is OK for bgplgsh (show ip bgp in "Peer 1" feels OK) but not for bgplg as I
> have to quote the peer description in the input box (feels rather unnatural).

it feels like allowing " in allowed characters for the CGI is starting
to open a can of worms..

personally I think I'd change the descriptions to avoid spaces.



Re: bgpd, use correct size for ASPATH_HEADER_SIZE

2018-10-25 Thread Denis Fondras
On Thu, Oct 25, 2018 at 10:57:58AM +0200, Claudio Jeker wrote:
> Currently struct aspath is defined with a placeholder for the dynamic data
> part.
> struct aspath {
> LIST_ENTRY(aspath)  entry;
> int refcnt; /* reference count */
> u_int16_t   len;/* total length of aspath in octets */
> u_int16_t   ascnt;  /* number of AS hops in data */
> u_char  data[1]; /* placeholder for actual data */
> };
> 
> The size of the struct - this placeholder was calculated as
> ASPATH_HEADER_SIZE using (sizeof(struct aspath) - sizeof(u_char)).
> Now that does not consider any padding bytes added. Instead this should
> use offsetof(struct aspath, data) so that the malloc does not allocate too
> much memory.
> 

OK denis@

> -- 
> :wq Claudio
> 
> Index: rde.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.198
> diff -u -p -r1.198 rde.h
> --- rde.h 24 Oct 2018 08:26:37 -  1.198
> +++ rde.h 25 Oct 2018 08:48:38 -
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "bgpd.h"
>  #include "log.h"
> @@ -125,7 +126,7 @@ struct rde_peer {
>  #define AS_SEQUENCE  2
>  #define AS_CONFED_SEQUENCE   3
>  #define AS_CONFED_SET4
> -#define ASPATH_HEADER_SIZE   (sizeof(struct aspath) - sizeof(u_char))
> +#define ASPATH_HEADER_SIZE   (offsetof(struct aspath, data))
>  
>  struct aspath {
>   LIST_ENTRY(aspath)  entry;
> 



/bin/df align inode output

2018-10-25 Thread Solene Rapenne
the following diff makes df printing aligned inode informations.

before patch

solene@t480 /usr/src/bin/df $ df -ik
Filesystem  1K-blocks  Used Avail Capacity iused   ifree  %iused  
Mounted on
/dev/sd2a 102887813786283957414%2227  153675 1%   /
/dev/sd2l   312080984  38691660 25778527613%  519480 19221190 3%   /home
/dev/sd2d 4125390  7718   3911404 0% 113  545549 0%   /tmp
/dev/sd2f 2061054786084   117191840%   14608  271214 5%   /usr
/dev/sd2g 102887819105278638420%9211  146691 6%   
/usr/X11R6
/dev/sd2h10318462   7347916   245462475%  171668 115351413%   
/usr/local
/dev/sd2k 6189758150842   5729430 3%2469  803033 0%   
/usr/obj
/dev/sd2j 2061054   111567884232457%  105820  18000237%   
/usr/src
/dev/sd2e20124398   1498948  17619232 8%   14606 2557808 1%   /var
/dev/sd2m   127381766  92115060  2889761876%  623330 15616668 4%   /data

with patch

solene@t480 /usr/src/bin/df $ ./df -ik
Filesystem  1K-blocks  Used Avail Capacity  iusedifree %iused  
Mounted on
/dev/sd2a 102887813786283957414% 2227   153675 1%   /
/dev/sd2l   312080984  38691660 25778527613%   519480 19221190 3%   
/home
/dev/sd2d 4125390  7718   3911404 0%  113   545549 0%   /tmp
/dev/sd2f 2061054786084   117191840%14608   271214 5%   /usr
/dev/sd2g 102887819105278638420% 9211   146691 6%   
/usr/X11R6
/dev/sd2h10318462   7347916   245462475%   171668  115351413%   
/usr/local
/dev/sd2k 6189758150842   5729430 3% 2469   803033 0%   
/usr/obj
/dev/sd2j 2061054   111567884232457%   105820   18000237%   
/usr/src
/dev/sd2e20124398   1498948  17619232 8%14606  2557808 1%   /var
/dev/sd2m   127381766  92115060  2889761876%   623330 15616668 4%   
/data


Index: df.c
===
RCS file: /cvs/src/bin/df/df.c,v
retrieving revision 1.59
diff -u -p -r1.59 df.c
--- df.c14 Aug 2016 21:07:40 -  1.59
+++ df.c25 Oct 2018 10:52:21 -
@@ -328,7 +328,7 @@ prtstat(struct statfs *sfsp, int maxwidt
if (iflag) {
inodes = sfsp->f_files;
used = inodes - sfsp->f_ffree;
-   (void)printf(" %7llu %7llu %5.0f%% ", used, sfsp->f_ffree,
+   (void)printf(" %8llu %8llu %5.0f%% ", used, sfsp->f_ffree,
   inodes == 0 ? 100.0 : (double)used / (double)inodes * 100.0);
} else
(void)printf("  ");
@@ -363,7 +363,7 @@ bsdprint(struct statfs *mntbuf, long mnt
 maxwidth, maxwidth, "Filesystem", header);
}
if (iflag)
-   (void)printf(" iused   ifree  %%iused");
+   (void)printf("  iusedifree %%iused");
(void)printf("  Mounted on\n");
 
 



unveil getconf

2018-10-25 Thread Ricardo Mestre
Hi,

The code path were we pass `pathname' in the arguments is already limited
with pledge(2), but since we know exactly what it is then we can go further and
also unveil(2) it with read permissions.

Comments? OK?

Index: getconf.c
===
RCS file: /cvs/src/usr.bin/getconf/getconf.c,v
retrieving revision 1.19
diff -u -p -u -r1.19 getconf.c
--- getconf.c   28 Oct 2016 07:22:59 -  1.19
+++ getconf.c   25 Oct 2018 10:12:31 -
@@ -513,6 +513,8 @@ main(int argc, char *argv[])
break;
 
case PATHCONF:
+   if (unveil(argv[1], "r") == -1)
+   err(1, "unveil");
if (pledge("stdio rpath", NULL) == -1)
err(1, "pledge");
errno = 0;



unveil kvm_mkdb

2018-10-25 Thread Ricardo Mestre
Hi,

If we pass `file' via args then we need to unveil(2) it with read permission,
otherwise if omitted we need to unveil(2) both _PATH_UNIX and _PATH_KSYMS with
same permissions.

Unconditionally we need to also unveil(2) dbdir, which by default is
_PATH_VARDB but can be changed via args (-o directory), with read/write/create
permissions. There are a couple of temp files that will be created but it's
inside dbdir so there's no need to unveil(2) them individually.

Since we already call pledge(2) before, twice, we need to add "unveil" promise
to both of them, and finally call pledge(2) once again with the needed promises
except "unveil".

Comments? OK?

Index: kvm_mkdb.c
===
RCS file: /cvs/src/usr.sbin/kvm_mkdb/kvm_mkdb.c,v
retrieving revision 1.29
diff -u -p -u -r1.29 kvm_mkdb.c
--- kvm_mkdb.c  21 Nov 2017 12:07:00 -  1.29
+++ kvm_mkdb.c  25 Oct 2018 09:45:31 -
@@ -70,7 +70,7 @@ main(int argc, char *argv[])
char *nlistpath, *nlistname;
char dbdir[PATH_MAX];
 
-   if (pledge("stdio rpath wpath cpath fattr getpw flock id", NULL) == -1)
+   if (pledge("stdio rpath wpath cpath fattr getpw flock id unveil", NULL) 
== -1)
err(1, "pledge");
 
/* Try to use the kmem group to be able to fchown() in kvm_mkdb(). */
@@ -89,7 +89,7 @@ main(int argc, char *argv[])
warn("can't set rlimit data size");
}
 
-   if (pledge("stdio rpath wpath cpath fattr flock", NULL) == -1)
+   if (pledge("stdio rpath wpath cpath fattr flock unveil", NULL) == -1)
err(1, "pledge");
 
strlcpy(dbdir, _PATH_VARDB, sizeof(dbdir));
@@ -114,6 +114,20 @@ main(int argc, char *argv[])
 
if (argc > 1)
usage();
+
+   if (argc > 0) {
+   if (unveil(argv[0], "r") == -1)
+   err(1, "unveil");
+   } else {
+   if (unveil(_PATH_UNIX, "r") == -1)
+   err(1, "unveil");
+   if (unveil(_PATH_KSYMS, "r") == -1)
+   err(1, "unveil");
+   }
+   if (unveil(dbdir, "rwc") == -1)
+   err(1, "unveil");
+   if (pledge("stdio rpath wpath cpath fattr flock", NULL) == -1)
+   err(1, "pledge");
 
/* If no kernel specified use _PATH_KSYMS and fall back to _PATH_UNIX */
if (argc > 0) {



bgpd, use correct size for ASPATH_HEADER_SIZE

2018-10-25 Thread Claudio Jeker
Currently struct aspath is defined with a placeholder for the dynamic data
part.
struct aspath {
LIST_ENTRY(aspath)  entry;
int refcnt; /* reference count */
u_int16_t   len;/* total length of aspath in octets */
u_int16_t   ascnt;  /* number of AS hops in data */
u_char  data[1]; /* placeholder for actual data */
};

The size of the struct - this placeholder was calculated as
ASPATH_HEADER_SIZE using (sizeof(struct aspath) - sizeof(u_char)).
Now that does not consider any padding bytes added. Instead this should
use offsetof(struct aspath, data) so that the malloc does not allocate too
much memory.

-- 
:wq Claudio

Index: rde.h
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.198
diff -u -p -r1.198 rde.h
--- rde.h   24 Oct 2018 08:26:37 -  1.198
+++ rde.h   25 Oct 2018 08:48:38 -
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "bgpd.h"
 #include "log.h"
@@ -125,7 +126,7 @@ struct rde_peer {
 #define AS_SEQUENCE2
 #define AS_CONFED_SEQUENCE 3
 #define AS_CONFED_SET  4
-#define ASPATH_HEADER_SIZE (sizeof(struct aspath) - sizeof(u_char))
+#define ASPATH_HEADER_SIZE (offsetof(struct aspath, data))
 
 struct aspath {
LIST_ENTRY(aspath)  entry;



Re: unveil bdftopcf

2018-10-25 Thread Ricardo Mestre
Something like this then?

If it's too much burden to keep these local patches I can drop it, no problem.

Index: bdftopcf.c
===
RCS file: /cvs/xenocara/app/bdftopcf/bdftopcf.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 bdftopcf.c
--- bdftopcf.c  29 Mar 2018 20:34:30 -  1.5
+++ bdftopcf.c  25 Oct 2018 07:00:50 -
@@ -39,6 +39,7 @@ from The Open Group.
 #include "bdfint.h"
 #include "pcf.h"
 #include 
+#include 
 #include 
 
 int
@@ -158,6 +159,38 @@ main(int argc, char *argv[])
 }
 argv++;
 }
+
+if (input_name) {
+if (unveil(input_name, "r") == -1) {
+fprintf(stderr, "%s: could not unveil %s\n",
+program_name, input_name);
+exit(1);
+   }
+}
+if (output_name) {
+if (unveil(output_name, "rwc") == -1) {
+fprintf(stderr, "%s: could not unveil %s\n",
+program_name, output_name);
+exit(1);
+}
+if (pledge("stdio rpath wpath cpath", NULL) == -1) {
+fprintf(stderr, "%s: could not pledge", program_name);
+exit(1);
+}
+}
+if (input_name && !output_name) {
+if (pledge("stdio rpath", NULL) == -1) {
+fprintf(stderr, "%s: could not pledge", program_name);
+exit(1);
+}
+}
+if (!input_name && !output_name) {
+if (pledge("stdio", NULL) == -1) {
+fprintf(stderr, "%s: could not pledge", program_name);
+exit(1);
+}
+}
+
 if (input_name) {
 input = FontFileOpen(input_name);
 if (!input) {

On 10:41 Wed 24 Oct , Theo de Raadt wrote:
> Matthieu Herrb  wrote:
> 
> > Generally, I'm not too found of pledging/unveiling random X client
> > programs. There are a lot of "hidden" features in X libraries that
> > will probably break with too strict pledges and/or unveils.
> 
> Well eventually we want to see if something can be done about xterm.
> Especially if the lessons learned (I suspect some hoisting will occur)
> can be pushed back upstream, and maybe allow others to apply their
> own system call limiter mechanism.  Perhaps not possible...
> 
> > Also since this is OpenBSD-specific, it will be difficult to get it
> > upstreams, especially if you don't provide the autoconf goo to make
> > the code still build/work on Linux. And when not upstreaming it
> > creates more burden to merge new versions of the applications.
> 
> Well, I doubt it will create too much burden, generally these unveil
> or pledge chunks are a small set of + lines, without changing other
> logic.
> 
> Anyways, bdftopcf is not running near a security boundary.
> 



bgpd: replace some more walkers with rib_dump

2018-10-25 Thread Claudio Jeker
Next step on my quest to make the RIB code better.
This changes the following things:
- network_flush is now using rib_dump_new to walk the Adj-RIB-In and
  remove all dynamically added announcements
- peer_flush got generalized and is now used also in peer_down.
  It also uses a rib_dump_new call to walk the Adj-RIB-In and remove
  all prefixes from a peer but this is done synchronous for now.
- peer_flush is now working correctly for AID_UNSPEC.
- Change the rib_valid check to continue instead of break out of the loop.
  The rib table can have holes so the loop needs to continue.

This removes the last three places that use the peer -> path -> prefix
tailqs so the next step is to remove these and make rde_aspath just an
object that is referenced by prefixes. After that adding a proper
Adj-RIB-Out should be possible.

Running with this on production :)
-- 
:wq Claudio

Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.440
diff -u -p -r1.440 rde.c
--- rde.c   24 Oct 2018 08:26:37 -  1.440
+++ rde.c   25 Oct 2018 06:34:32 -
@@ -97,7 +97,7 @@ struct rde_peer   *peer_add(u_int32_t, str
 struct rde_peer*peer_get(u_int32_t);
 voidpeer_up(u_int32_t, struct session_up *);
 voidpeer_down(u_int32_t);
-voidpeer_flush(struct rde_peer *, u_int8_t);
+voidpeer_flush(struct rde_peer *, u_int8_t, time_t);
 voidpeer_stale(u_int32_t, u_int8_t);
 voidpeer_dump(u_int32_t, u_int8_t);
 static void peer_recv_eor(struct rde_peer *, u_int8_t);
@@ -105,7 +105,8 @@ static void  peer_send_eor(struct rde_pe
 
 voidnetwork_add(struct network_config *, int);
 voidnetwork_delete(struct network_config *, int);
-voidnetwork_dump_upcall(struct rib_entry *, void *);
+static void network_dump_upcall(struct rib_entry *, void *);
+static void network_flush_upcall(struct rib_entry *, void *);
 
 voidrde_shutdown(void);
 int sa_cmp(struct bgpd_addr *, struct sockaddr *);
@@ -418,7 +419,7 @@ rde_dispatch_imsg_session(struct imsgbuf
imsg.hdr.peerid);
break;
}
-   peer_flush(peer, aid);
+   peer_flush(peer, aid, peer->staletime[aid]);
break;
case IMSG_SESSION_RESTARTED:
if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(aid)) {
@@ -434,7 +435,7 @@ rde_dispatch_imsg_session(struct imsgbuf
break;
}
if (peer->staletime[aid])
-   peer_flush(peer, aid);
+   peer_flush(peer, aid, peer->staletime[aid]);
break;
case IMSG_REFRESH:
if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(aid)) {
@@ -556,7 +557,10 @@ badnetdel:
log_warnx("rde_dispatch: wrong imsg len");
break;
}
-   prefix_network_clean(peerself);
+   if (rib_dump_new(RIB_ADJ_IN, AID_UNSPEC,
+   RDE_RUNNER_ROUNDS, peerself, network_flush_upcall,
+   NULL, NULL) == -1)
+   log_warn("rde_dispatch: IMSG_NETWORK_FLUSH");
break;
case IMSG_FILTER_SET:
if (imsg.hdr.len - IMSG_HEADER_SIZE !=
@@ -770,7 +774,7 @@ rde_dispatch_imsg_parent(struct imsgbuf 
memcpy(nconf, imsg.data, sizeof(struct bgpd_config));
for (rid = 0; rid < rib_size; rid++) {
if (!rib_valid(rid))
-   break;
+   continue;
ribs[rid].state = RECONF_DELETE;
}
SIMPLEQ_INIT(>rde_prefixsets);
@@ -1411,7 +1415,7 @@ rde_update_update(struct rde_peer *peer,
 
for (i = RIB_LOC_START; i < rib_size; i++) {
if (!rib_valid(i))
-   break;
+   continue;
rde_filterstate_prep(, >aspath, in->nexthop,
in->nhflags);
/* input filter */
@@ -1443,7 +1447,7 @@ rde_update_withdraw(struct rde_peer *pee
 
for (i = RIB_LOC_START; i < rib_size; i++) {
if (!rib_valid(i))
-   break;
+   continue;
if (prefix_remove([i].rib, peer, prefix, prefixlen))
rde_update_log("withdraw", i, peer, NULL, prefix,
prefixlen);
@@ -2957,11 +2961,10 @@ rde_reload_done(void)
 static void
 

Re: Fix descriptions of smtps vs smtp+tls in smtpd.conf.5

2018-10-25 Thread Gilles Chehade
On Thu, Oct 25, 2018 at 07:24:33AM +0100, Raf Czlonka wrote:
> On Thu, Oct 25, 2018 at 07:11:47AM BST, Gilles Chehade wrote:
> > 
> > smtpd will _always_ display a 'starttls' log line when the TLS channel 
> > starts,
> > disregarding if TLS was started at connect time (smtps) or within the 
> > protocol
> > (smtp+tls, or even smtp since it does opportunistic tls).
> > 
> 
> I guess this is the confusing bit - seeing 'starttls' in the log
> file and thinking 'STARTTLS', i.e. the "TLS upgrade".
> 

yes, maybe it should just display 'tls' instead of 'starttls'


-- 
Gilles Chehade

https://www.poolp.org  @poolpOrg



Re: Fix descriptions of smtps vs smtp+tls in smtpd.conf.5

2018-10-25 Thread Raf Czlonka
On Thu, Oct 25, 2018 at 07:11:47AM BST, Gilles Chehade wrote:
> 
> smtpd will _always_ display a 'starttls' log line when the TLS channel starts,
> disregarding if TLS was started at connect time (smtps) or within the protocol
> (smtp+tls, or even smtp since it does opportunistic tls).
> 

I guess this is the confusing bit - seeing 'starttls' in the log
file and thinking 'STARTTLS', i.e. the "TLS upgrade".

R.



Re: Fix descriptions of smtps vs smtp+tls in smtpd.conf.5

2018-10-25 Thread Gilles Chehade
On Mon, Oct 22, 2018 at 08:37:25PM -0400, trondd wrote:
> Unless I'm confused, it seems the description of the smarthosts smtps and
> smtp+tls are revered in the smtpd.conf man page.
>

You are confused ;-)


> My log seemed to back this up.  When using smtp+tls, which the man page said
> uses STARTTLS but seems to actually use TLS which my ISP does not:
> 
> Oct 21 21:42:58 ember smtpd[41596]: ca9dba5e7f80e6ca mta connecting 
> address=smtp+tls://68.87.20.6:465 host=omta-ch2.sys.comcast.net
> Oct 21 21:42:58 ember smtpd[41596]: ca9dba5e7f80e6ca mta connected
> Oct 21 21:43:59 ember smtpd[41596]: ca9dba5e7f80e6ca mta error 
> reason=Connection closed unexpectedly
> 

You are mistaking smtps and smtp+tls:

In an smtps session, the TLS negotation takes place during the connection so
client and server are already in a secure channel when the SMTP session gets
started.

In a smtp+tls session, the TLS negotiation takes place after the session has
started in plaintext through the use of the STARTTLS SMTP extension.

In your example here, you are using smtp+tls on a host that expects smtps so
the TLS negotation can't play out and you're kicked out.


> And with smtps, which the man page said uses TLS, logs show STARTTLS:
> 
> Oct 21 22:02:06 ember smtpd[66745]: a9193b70dbc40df0 mta connecting 
> address=smtps://68.87.20.6:465 host=omta-ch2.sys.comcast.net
> Oct 21 22:02:06 ember smtpd[66745]: a9193b70dbc40df0 mta connected
> Oct 21 22:02:06 ember smtpd[66745]: a9193b70dbc40df0 mta starttls 
> ciphers=version=TLSv1.2, cipher=ECDHE-RSA-AES256-GCM-SHA384, bits=256
> Oct 21 22:02:06 ember smtpd[66745]: smtp-out: Server certificate verification 
> succeeded on session a9193b70dbc40df0
> 

TLS and STARTTLS are essentially the same as far as you're concerned.

smtpd will _always_ display a 'starttls' log line when the TLS channel starts,
disregarding if TLS was started at connect time (smtps) or within the protocol
(smtp+tls, or even smtp since it does opportunistic tls).

The only issue here is that you attempted to connect in plaintext then upgrade
a session on a host that didn't speak plaintext and expected sessions to speak
TLS from the start.

-- 
Gilles Chehade

https://www.poolp.org  @poolpOrg