dhcrelay bpf bugs

[David Gwynne]

there are two bugs in the bpf code in dhcrelay. they're obviously
rare though.

the worst one is when skipping a packet, dhcrelay uses the = operator
instead of + when summing the bpf header and capture length.

the second bug is also in calculating how much space a packet
consumes to skip over it. bpf aligns packets to a bpf word boundary,
but reports buffer lenghts in bytes. a program has to calculate the
next packet boundary with these values, but dhcrelay isnt. the
result of this would be dhcrelay using packet data and the wrong
fields in a bpf header, which is bad.

the fix for this is to calculate the offset to the next packet once,
and add that to the offset when skipping a packet. the code currently
skips the bpf header to get at the packet, which is now changed so
the packet start is accounted separately.

ive only tested that the code still works, i havent come up with a
test for the cases this diff fixes. ok?

ps, i think the whole code smells a bit, but i like the free diffz.

Index: bpf.c
===
RCS file: /cvs/src/usr.sbin/dhcrelay/bpf.c,v
retrieving revision 1.19
diff -u -p -r1.19 bpf.c
--- bpf.c   19 Apr 2017 05:36:12 -  1.19
+++ bpf.c   24 Nov 2017 02:10:48 -
@@ -375,6 +375,7 @@ receive_packet(struct interface_info *in
int length = 0;
ssize_t offset = 0;
struct bpf_hdr hdr;
+   size_t bpflen, pktoff;
 
/*
 * All this complexity is because BPF doesn't guarantee that
@@ -412,12 +413,14 @@ receive_packet(struct interface_info *in
memcpy(, >rbuf[interface->rbuf_offset],
sizeof(hdr));
 
+   bpflen = BPF_WORDALIGN(hdr.bh_hdrlen + hdr.bh_caplen);
+   pktoff = interface->rbuf_offset + hdr.bh_hdrlen;
+
/*
 * If the bpf header plus data doesn't fit in what's
 * left of the buffer, stick head in sand yet again...
 */
-   if (interface->rbuf_offset + hdr.bh_hdrlen + hdr.bh_caplen >
-   interface->rbuf_len) {
+   if (interface->rbuf_offset + bpflen > interface->rbuf_len) {
interface->rbuf_offset = interface->rbuf_len;
continue;
}
@@ -427,18 +430,14 @@ receive_packet(struct interface_info *in
 * the packet won't fit in the input buffer, all we can
 * do is drop it.
 */
-   if (hdr.bh_caplen != hdr.bh_datalen) {
-   interface->rbuf_offset += hdr.bh_hdrlen =
-   hdr.bh_caplen;
+   if (hdr.bh_caplen < hdr.bh_datalen) {
+   interface->rbuf_offset += bpflen;
continue;
}
 
-   /* Skip over the BPF header... */
-   interface->rbuf_offset += hdr.bh_hdrlen;
-
/* Decode the physical header... */
offset = decode_hw_header(interface->rbuf,
-   interface->rbuf_len, interface->rbuf_offset, pc,
+   interface->rbuf_len, pktoff, pc,
interface->hw_address.htype);
 
/*
@@ -447,7 +446,7 @@ receive_packet(struct interface_info *in
 * skip this packet.
 */
if (offset < 0) {
-   interface->rbuf_offset += hdr.bh_caplen;
+   interface->rbuf_offset += bpflen;
continue;
}
 
@@ -457,27 +456,23 @@ receive_packet(struct interface_info *in
 
/* If the IP or UDP checksum was bad, skip the packet... */
if (offset < 0) {
-   interface->rbuf_offset += hdr.bh_caplen;
+   interface->rbuf_offset += bpflen;
continue;
}
 
-   hdr.bh_caplen -= offset - interface->rbuf_offset;
-   interface->rbuf_offset = offset;
-
/*
 * If there's not enough room to stash the packet data,
 * we have to skip it (this shouldn't happen in real
 * life, though).
 */
if (hdr.bh_caplen > len) {
-   interface->rbuf_offset += hdr.bh_caplen;
+   interface->rbuf_offset += bpflen;
continue;
}
 
/* Copy out the data in the packet... */
-   memcpy(buf, interface->rbuf + interface->rbuf_offset,
-   hdr.bh_caplen);
-   interface->rbuf_offset += hdr.bh_caplen;
+   memcpy(buf, interface->rbuf + pktoff, hdr.bh_caplen);
+   interface->rbuf_offset += bpflen;
return (hdr.bh_caplen);
} while (!length);
return (0);

pfctl and anchors: few more glitches found

[Alexandr Nedvedicky]

Hello,

patch below fixes various glitches I've found in pfctl. All issues are related
to anchors. I did test the patch using regress/sbin/pfctl. Although all tests
have passed, I still would like to ask for testing in field, especially if you
use some fancy configuration with 'load anchor'. The patch is just intended for
testing. I'll break the patch to smaller chunks to get some OKs.

the list of bugfixes is as follows:

- pfctl_optimize.c is not able to create radix table. This bug was reported
  by Leonardo Guardati. It's yet another occurrence of 'name vs. path' mix
  up [1]

- use after free, when pfctl_optimize fails to create radix table.  We are
  just (un)lucky enough the Leonardo hit [1] in pfctl_optimize.c

- the use after free is triggered in error path of
  pfctl_optimize_ruleset(). I gave a closer look to function and spot a
  memory leak. Fortunately the memory leak is not critical, because the
  pfctl is just one-shot application: load rules and quit anyway.

- when I implemented a test case using the rules from Leonardo I found
  yet another oddity in processing of 'load anchor'. The 'load anchor"
  is handled differently when the rules are being loaded to root
  anchor (pfctl -f pf-load-anchor.conf) and when the same rules are being
  loaded to non-root anchor (pfctl -a regression -f pf-load-anchor.conf).

The test case I've got from Leonardo is simple configuration using nested
anchors. There are just two nesting levels. The pf.conf file, which loads
level one looks as follows:
8<---8<---8<--8<
anchor "one"
load anchor "one" from "/home/sashan/leonardo/pf-optimize.one"
8<---8<---8<--8<

the pf-optimize.one file contains just simple anchor, which loads anchor
two:
8<---8<---8<--8<
anchor "two"
load anchor "two" from "/home/sashan/leonardo/pf-optimize.two"
8<---8<---8<--8<

finally the pf-optimize.two contains a ruleset, which triggers the
bugs found in pfctl_optimize.c:
8<---8<---8<--8<
addrs = "{  1.2.3.4,
10.20.30.40,
2.4.6.8,
20.40.60.80,
4.8.12.16,
40.80.120.160,
5.6.7.8,
50.60.70.80,
10.12.14.16,
100.120.140.160
}"
pass from $addrs 
8<---8<---8<--8<
As you can see the optimizer is supposed to  fold the list of addresses 'addrs'
to radix table. This currently fails because of 'name vs. path' mix up
in call to pfctl_define_table().

Assuming the 'mix up' is either fixed (or pf-optimize.two is altered to simple
'pass all') we can load the pf.conf to driver using 'pfctl -f pf.conf' command:
8<---8<---8<--8<
pfctl -f pf.conf
pfctl -vsA
  one
  one/two
8<---8<---8<--8<
The output of 'pfctl -vsA' is something we expect. Now let's try to load
the pf.conf using 'pfctl -a regress -f pf.conf' we get something like this:
8<---8<---8<--8<
pfctl -a regress -f pf.conf
pfctl -vsA  

  
  regress
  regress/one
8<---8<---8<--8<
we are missing 'regress/one/two' anchor in output above. With patch applied we
get expected results:
8<---8<---8<--8<
pfctl -a regress -f /home/sashan/leonardo/pf-optimize.conf  

  
pfctl -vsA
  regress
  regress/one
  regress/one/two
8<---8<---8<--8<

I'll send individual fixes for review later on Friday.

Thank you for testing of patch below.

regards
sasha


[1] https://marc.info/?l=openbsd-tech=147292142630058=2

8<---8<---8<--8<
diff --git a/regress/sbin/pfctl/Makefile b/regress/sbin/pfctl/Makefile
index d42bb446e68..5572785b121 100644
--- a/regress/sbin/pfctl/Makefile
+++ b/regress/sbin/pfctl/Makefile
@@ -30,6 +30,7 @@ PFIF2IP=1 2 3
 PFCHKSUM=1 2 3
 PFCMD=1
 PFCMDFAIL=1
+PFLOADANCHORS=112 113
 
 MAKEOBJDIRPREFIX=
 
@@ -328,6 +329,20 @@ pfchksum-update:   ${PFCHKSUM_UPDATES}
 NODEFAULT_TARGETS+=pfchksum
 REGRESS_ROOT_TARGETS+=pfchksum
 
+.for n in ${PFLOADANCHORS}
+PFLOADANCHORS_TARGETS+=pfloadanchors${n}
+
+pfloadanchors${n}:
+   ${SUDO} pfctl -Fa > 

Re: isakmpd.8: define "SA" abbreviation before use

[Jason McIntyre]

On Thu, Nov 23, 2017 at 01:19:51PM -0600, Scott Cheloha wrote:
> Hi,
> 
> This makes parts of isakmpd(8) more immediately intelligible to
> a beginner.
> 
> --
> Scott Cheloha
> 
> Index: sbin/isakmpd/isakmpd.8
> ===
> RCS file: /cvs/src/sbin/isakmpd/isakmpd.8,v
> retrieving revision 1.118
> diff -u -p -r1.118 isakmpd.8
> --- sbin/isakmpd/isakmpd.85 Mar 2016 08:38:36 -   1.118
> +++ sbin/isakmpd/isakmpd.823 Nov 2017 19:25:20 -
> @@ -50,7 +50,7 @@
>  .Sh DESCRIPTION
>  The
>  .Nm
> -daemon establishes security associations for encrypted
> +daemon establishes security associations (SAs) for encrypted
>  and/or authenticated network traffic.
>  At this moment, and probably forever, this means
>  .Xr ipsec 4
> 

i think that's reasonable. i started to make the change, then found
ipsec.conf.5 needed adjusting, and then finally found the worm can: we
spell both "Security Association" and "security association" in the
manuals. there's too many for me to try and roll in, so i committed the
diff below as a best i can do now. not sure if it's worth changing
20-odd examples one way or the other...

thanks for your mail,
jmc

Index: ipsecctl/ipsec.conf.5
===
RCS file: /cvs/src/sbin/ipsecctl/ipsec.conf.5,v
retrieving revision 1.153
diff -u -r1.153 ipsec.conf.5
--- ipsecctl/ipsec.conf.5   27 Oct 2017 08:29:32 -  1.153
+++ ipsecctl/ipsec.conf.5   23 Nov 2017 20:47:08 -
@@ -44,9 +44,7 @@
 In its most basic form, a
 .Em flow
 is established between hosts and/or networks,
-and then Security Associations
-.Pq Em SA
-are established,
+and then Security Associations (SAs) are established,
 which detail how the desired protection will be achieved.
 IPsec uses flows
 to determine whether to apply security services to an IP packet or not.
Index: isakmpd/isakmpd.8
===
RCS file: /cvs/src/sbin/isakmpd/isakmpd.8,v
retrieving revision 1.118
diff -u -r1.118 isakmpd.8
--- isakmpd/isakmpd.8   5 Mar 2016 08:38:36 -   1.118
+++ isakmpd/isakmpd.8   23 Nov 2017 20:47:08 -
@@ -50,7 +50,7 @@
 .Sh DESCRIPTION
 The
 .Nm
-daemon establishes security associations for encrypted
+daemon establishes Security Associations (SAs) for encrypted
 and/or authenticated network traffic.
 At this moment, and probably forever, this means
 .Xr ipsec 4
@@ -212,8 +212,7 @@
 .It Fl f Ar fifo
 The
 .Fl f
-option specifies the
-.Tn FIFO
+option specifies the FIFO
 (a.k.a. named pipe) where the daemon listens for
 user requests.
 If the path given is a dash

isakmpd.8: define "SA" abbreviation before use

[Scott Cheloha]

Hi,

This makes parts of isakmpd(8) more immediately intelligible to
a beginner.

--
Scott Cheloha

Index: sbin/isakmpd/isakmpd.8
===
RCS file: /cvs/src/sbin/isakmpd/isakmpd.8,v
retrieving revision 1.118
diff -u -p -r1.118 isakmpd.8
--- sbin/isakmpd/isakmpd.8  5 Mar 2016 08:38:36 -   1.118
+++ sbin/isakmpd/isakmpd.8  23 Nov 2017 19:25:20 -
@@ -50,7 +50,7 @@
 .Sh DESCRIPTION
 The
 .Nm
-daemon establishes security associations for encrypted
+daemon establishes security associations (SAs) for encrypted
 and/or authenticated network traffic.
 At this moment, and probably forever, this means
 .Xr ipsec 4

Re: Fix segfault on vi(1)

[Martijn van Duren]

OK martijn@

On 11/23/17 14:14, Ricardo Mestre wrote:
> Index: common/delete.c
> ===
> RCS file: /cvs/src/usr.bin/vi/common/delete.c,v
> retrieving revision 1.11
> diff -u -p -u -r1.11 delete.c
> --- common/delete.c   6 Jan 2016 22:28:52 -   1.11
> +++ common/delete.c   23 Nov 2017 12:52:38 -
> @@ -87,14 +87,16 @@ del(SCR *sp, MARK *fm, MARK *tm, int lmo
>   if (tm->lno == fm->lno) {
>   if (db_get(sp, fm->lno, DBG_FATAL, , ))
>   return (1);
> - GET_SPACE_RET(sp, bp, blen, len);
> - if (fm->cno != 0)
> - memcpy(bp, p, fm->cno);
> - memcpy(bp + fm->cno, p + (tm->cno + 1), len - (tm->cno + 1));
> - if (db_set(sp, fm->lno,
> - bp, len - ((tm->cno - fm->cno) + 1)))
> - goto err;
> - goto done;
> + if (len != 0) {
> + GET_SPACE_RET(sp, bp, blen, len);
> + if (fm->cno != 0)
> + memcpy(bp, p, fm->cno);
> + memcpy(bp + fm->cno, p + (tm->cno + 1), len - (tm->cno 
> + 1));
> + if (db_set(sp, fm->lno,
> + bp, len - ((tm->cno - fm->cno) + 1)))
> + goto err;
> + goto done;
> + }
>   }
>  
>   /*
> 

Re: race-less nd6_timer

[Alexander Bluhm]

On Wed, Nov 22, 2017 at 04:24:22PM +0100, Martin Pieuchot wrote:
> Diff below implements 3/ because it seems the simplest approach to
> me and reduce differences with ARP a bit further.

Yes.

>  void
> -nd6_llinfo_settimer(struct llinfo_nd6 *ln, int secs)
> +nd6_llinfo_settimer(struct llinfo_nd6 *ln, unsigned int secs)
>  {
> - if (secs < 0) {
> - ln->ln_rt->rt_expire = 0;
> - timeout_del(>ln_timer_ch);
> - } else {
> - ln->ln_rt->rt_expire = time_uptime + secs;
> - timeout_add_sec(>ln_timer_ch, secs);
> + time_t expire = time_uptime + secs;
> +
> + ln->ln_rt->rt_expire = expire;
> + if (!timeout_pending(_timer_to) || expire < nd6_timer_next) {
> + nd6_timer_next = expire;
> + timeout_add_sec(_timer_to, secs);
>   }
>  }

The global nd6_timer_next is protected by the net lock.  Should we
put an NET_ASSERT_LOCKED() into nd6_llinfo_settimer()?

OK bluhm@

Re: fuse: vfs create does not map 1:1 to fuse create

[Helg]

On Thu, Nov 23, 2017 at 12:09:34PM +, Helg Bredow wrote:
> - Forwarded message from Martin Pieuchot  -
> 
> Date: Sat, 18 Nov 2017 11:03:49 +0100
> From: Martin Pieuchot 
> To: Helg Bredow 
> CC: "tech@openbsd.org" 
> Subject: Re: fuse: vfs create does not map 1:1 to fuse create
> User-Agent: Mutt/1.9.1 (2017-09-22)
> 
> On 18/11/17(Sat) 02:22, Helg Bredow wrote:
> > On Fri, 10 Nov 2017 09:09:32 +0100
> > Martin Pieuchot  wrote:
> > 
> > > On 09/11/17(Thu) 01:20, Helg Bredow wrote:
> > > > > On 08/11/17(Wed) 14:12, Helg Bredow wrote:
> > > > > > There is a bug when creating a file in fuse-exfat and then deleting 
> > > > > > it
> > > > > > again without first unmounting the file system. The reason for this 
> > > > > > is
> > > > > > that fuse-exfat maintains strict reference counts and fuse currently
> > > > > > calls the file system create and open functions when it should only
> > > > > > call create. 
> > > > > > [...]
> > > > > 
> > [...]
> > > 
[...]
> > 
> > I now this caused some confusion but I believe the patch below is the
> > correct solution.
> 
> When you use the work "believing" you're asking us to trust you without
> argumenting.
> 
> >   Here's the mapping of file system calls so it's clear.
> > [...]
> 
> That's irrelevant.
> 
> There's no such thing as "atomic_open" from the userland point of view.
> So what you're discussing here is an "implementation details" of Linux
> VFS.
> 
> What matters is which syscall the application do.  I believe it is
> open(2).  So how its arguments are translated to the userland FS?  If
> I understand your diff correctly, OpenBSD will now do something like
> below (please correct the graph):
> 
> 
>   USER PROCESSfuse-zip
>   |   ^
>open(2)| 
>   |   FBT_MKNOD + FBT_OPEN 
>  \/ |
>   -
>   
> KERNEL
> 
> The only thing I understand is that you're changing the kernel behavior
> for all open(2) syscall operating on FUSE filesystem.  Before the kernel
> was generating a FBT_CREATE and FBT_OPEN messages, with some arguments.
> Now you want to generate FTB_MKNOD and FBT_OPEN with other arguments.
> 
> To me it sounds that you're working around a problem you don't
> understand.  If FUSE FSs want a FUSE 'create' operation then let's
> give them that!  What argument do they expect?
> 
> So let's start from the beginning:
> 
> - Which syscall is causing problem with which arguments?
> - Which FUSE operation(s) FUSE FSs are expecting for this exact syscall
>   with these arguments? 
> - Which FUSE operation(s) OpenBSD FUSE is currently generating with which
>   arguments?
 
The syscall that is causing the problem is open(2) with the O_CREAT flag.
With my change, this is the call graph.
 
 open(2)
   |
 vn_open()
   |
   +--VOP_CREATE(9) when O_CREAT
   |   |
   |   +-->fusefs_create()-->FBT_MKNOD-->ifuse_ops_mknod()
   |   |
   |   +--> fs mknod()
   |
   +--VOP_SETATTR(9) when O_TRUNC
   |   |
   |   +-->fusefs_setattr()-->FBT_SETATTR-->ifuse_ops_setattr()
   |   |
   |   +--> fs truncate()
   |
   +--VOP_OPEN(9)   always
   |
   +--fusefs_open()-->FBT_OPEN-->ifuse_ops_open()
   |
   +--> fs open()
 
 
The file systems are expecting the following arguments.

mknod(path, mode, dev)
   mode is that passed to open(2)
   dev will be 0 for regular files. FUSE mknod can create regular files.
open(path, fuse_file_info)
   fuse_file_info has (almost all) flags passed to open(2) plus other
   things that are not set by ifuse_ops_open. The FS will also return
   a handle to the file that was opened which it expects to receive
   again for functions that operate on the same file.
create(path, mode, fuse_file_info)
   mode is that passed to open(2)
   fuse_file_info has (almost all) flags passed to open(2) plus other
   things that are not set by ifuse_ops_open. The FS will also return
   a handle to the file that was opened which it expects to receive
   again for functions that operate on the same file.
   
OpenBSD FUSE is currently generating the following sequence of FUSE
calls when creating a file with open(2).

  FBT_CREATE + FBT_OPEN
  create arguments are hard-coded to O_CREAT | O_RDWR
  open arguments are either O_RDONLY, O_RDWR, O_WRONLY depending on
  mode

The problem is in vn_open(9). It does not pass the flags to VOP_CREATE
so we don't have them in fusefs_create(). We could defer calling create
when fusefs_open() is called but then we don't have the mode. We could
store the mode in fusefs_create() but 

Re: macppc: default to MBR for new installs

[Janne Johansson]

2017-11-22 23:20 GMT+01:00 Stefan Sperling :

> This flips the default response for the macppc disk layout question
> from HFS to MBR.
>
>
>
Make sure to edit INSTALL.macppc to reflect the new default,
the text seems to hint at the old default at various places, of course.

-- 
May the most significant bit of your life be positive.

Re: macppc: default to MBR for new installs

[Peter Hessler]

OK

On 2017 Nov 22 (Wed) at 23:20:46 +0100 (+0100), Stefan Sperling wrote:
:This flips the default response for the macppc disk layout question
:from HFS to MBR.
:
:I use an MBR on all my macppc machines. Booting OpenBSD is much simpler
:this way. I don't see why I cannot just hit enter for this question on
:new installs. I'd rather let Mac software archaeologists who wish to
:dual-boot an obsolete Mac OS with OpenBSD do the extra work.
:
:There is a follow-up question in md_prep_MBR which prints a warning
:and confirms this choice again. So I don't expect this change will
:cause anyone to overwrite an HFS partition table by accident.
:
:OK?
:
:Index: install.md
:===
:RCS file: /cvs/src/distrib/macppc/ramdisk/install.md,v
:retrieving revision 1.71
:diff -u -p -r1.71 install.md
:--- install.md 28 Jul 2017 18:15:44 -  1.71
:+++ install.md 22 Nov 2017 22:11:54 -
:@@ -144,7 +144,7 @@ md_prep_disklabel() {
:   PARTTABLE=
:   while [[ -z $PARTTABLE ]]; do
:   resp=MBR
:-  disk_has $_disk hfs && ask "Use HFS or MBR partition table?" HFS
:+  disk_has $_disk hfs && ask "Use HFS or MBR partition table?" MBR
:   case $resp in
:   [mM]*)  md_prep_MBR $_disk && PARTTABLE=MBR ;;
:   [hH]*)  md_prep_HFS $_disk && PARTTABLE=HFS ;;
:
:

-- 
There are times when truth is stranger than fiction and lunch time is
one of them.