Re: mg backup directory

[Lucas Gabriel Vuotto]

Sorry, space got mangled in previous email.

Index: fileio.c
===
RCS file: /cvs/src/usr.bin/mg/fileio.c,v
retrieving revision 1.103
diff -u -p -u -p -r1.103 fileio.c
--- fileio.c28 Jul 2016 21:40:25 -  1.103
+++ fileio.c13 May 2017 04:15:15 -
@@ -641,14 +641,15 @@ bkuplocation(const char *fn)
 int
 backuptohomedir(int f, int n)
 {
-   const char  *c = _PATH_MG_DIR;
-   char*p;
+   char*home;

if (bkupdir == NULL) {
-   p = adjustname(c, TRUE);
-   bkupdir = strndup(p, NFILEN);
-   if (bkupdir == NULL)
-   return(FALSE);
+   if ((home = getenv("HOME")) == NULL || *home == '\0')
+   return (FALSE);
+   if (asprintf(, _PATH_MG_DIR, home) == -1) {
+   bkupdir = NULL;
+   return (FALSE);
+   }

if (mkdir(bkupdir, 0700) == -1 && errno != EEXIST) {
free(bkupdir);
@@ -736,7 +737,7 @@ expandtilde(const char *fn)
plen = strlcpy(path, pw->pw_dir, sizeof(path));
if (plen == 0 || path[plen - 1] != '/') {
if (strlcat(path, "/", sizeof(path)) >= sizeof(path)) {
-   dobeep();   
+   dobeep();
ewprintf("Path too long");
return (NULL);
}
Index: pathnames.h
===
RCS file: /cvs/src/usr.bin/mg/pathnames.h,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 pathnames.h
--- pathnames.h 18 Jun 2012 07:14:55 -  1.1
+++ pathnames.h 13 May 2017 04:15:15 -
@@ -6,6 +6,6 @@
  * standard path names
  */

-#define_PATH_MG_DIR"~/.mg.d"
+#define_PATH_MG_DIR"%s/.mg.d"
 #define_PATH_MG_STARTUP"%s/.mg"
 #define_PATH_MG_TERM   "%s/.mg-%s"

On 13/05/17 01:25, Lucas Gabriel Vuotto wrote:
> Hi tech@,
> 
> mg(1)'s backup-to-home-directory writes backup files to `~/.mg.d'
> according to the manpage. In order to expand the tilde, it uses a
> custom function (expandtilde, fileio.c:700) which uses the pw entry for
> the user name returned by getlogin(2). This can lead to an undesired
> result if mg is run under another user via su.
> 
> For reading the startup file `~/.mg', it just tries to get HOME from
> environment and falls back to a default location defined at compile
> time if it can't get HOME. I think that it should do the same to
> setup the backup directory, with no fall back. Inlined is a patch
> to do that. While there, remove trailing spaces in fileio.c. A fall
> back could be added if needed. In that case I suggest using `/tmp'
> instead of a path defined at compile time.
> 
> If it isn't viable to merge the patch, I think the current behaviour
> should be reflected in the manpage. I can also write a patch for that.
> 
> OK?
> 
> Cheers.
> 
> Index: fileio.c
> ===
> RCS file: /cvs/src/usr.bin/mg/fileio.c,v
> retrieving revision 1.103
> diff -u -p -u -p -r1.103 fileio.c
> --- fileio.c28 Jul 2016 21:40:25 -1.103
> +++ fileio.c13 May 2017 04:15:15 -
> @@ -641,14 +641,15 @@ bkuplocation(const char *fn)
>  int
>  backuptohomedir(int f, int n)
>  {
> -const char*c = _PATH_MG_DIR;
> -char*p;
> +char*home;
> 
>  if (bkupdir == NULL) {
> -p = adjustname(c, TRUE);
> -bkupdir = strndup(p, NFILEN);
> -if (bkupdir == NULL)
> -return(FALSE);
> +if ((home = getenv("HOME")) == NULL || *home == '\0')
> +return (FALSE);
> +if (asprintf(, _PATH_MG_DIR, home) == -1) {
> +bkupdir = NULL;
> +return (FALSE);
> +}
> 
>  if (mkdir(bkupdir, 0700) == -1 && errno != EEXIST) {
>  free(bkupdir);
> @@ -736,7 +737,7 @@ expandtilde(const char *fn)
>  plen = strlcpy(path, pw->pw_dir, sizeof(path));
>  if (plen == 0 || path[plen - 1] != '/') {
>  if (strlcat(path, "/", sizeof(path)) >= sizeof(path)) {
> -dobeep();   
> +dobeep();
>  ewprintf("Path too long");
>  return (NULL);
>  }
> Index: pathnames.h
> ===
> RCS file: /cvs/src/usr.bin/mg/pathnames.h,v
> retrieving revision 1.1
> diff -u -p -u -p -r1.1 pathnames.h
> --- pathnames.h18 Jun 2012 07:14:55 -1.1
> +++ pathnames.h13 May 2017 04:15:15 -
> @@ -6,6 +6,6 @@
>   *standard path names
>   */
> 
> -#define_PATH_MG_DIR"~/.mg.d"
> +#define_PATH_MG_DIR"%s/.mg.d"
>  #define

mg backup directory

[Lucas Gabriel Vuotto]

Hi tech@,

mg(1)'s backup-to-home-directory writes backup files to `~/.mg.d'
according to the manpage. In order to expand the tilde, it uses a
custom function (expandtilde, fileio.c:700) which uses the pw entry for
the user name returned by getlogin(2). This can lead to an undesired
result if mg is run under another user via su.

For reading the startup file `~/.mg', it just tries to get HOME from
environment and falls back to a default location defined at compile
time if it can't get HOME. I think that it should do the same to
setup the backup directory, with no fall back. Inlined is a patch
to do that. While there, remove trailing spaces in fileio.c. A fall
back could be added if needed. In that case I suggest using `/tmp'
instead of a path defined at compile time.

If it isn't viable to merge the patch, I think the current behaviour
should be reflected in the manpage. I can also write a patch for that.

OK?

Cheers.

Index: fileio.c
===
RCS file: /cvs/src/usr.bin/mg/fileio.c,v
retrieving revision 1.103
diff -u -p -u -p -r1.103 fileio.c
--- fileio.c28 Jul 2016 21:40:25 -  1.103
+++ fileio.c13 May 2017 04:15:15 -
@@ -641,14 +641,15 @@ bkuplocation(const char *fn)
 int
 backuptohomedir(int f, int n)
 {
-   const char  *c = _PATH_MG_DIR;
-   char*p;
+   char*home;

if (bkupdir == NULL) {
-   p = adjustname(c, TRUE);
-   bkupdir = strndup(p, NFILEN);
-   if (bkupdir == NULL)
-   return(FALSE);
+   if ((home = getenv("HOME")) == NULL || *home == '\0')
+   return (FALSE);
+   if (asprintf(, _PATH_MG_DIR, home) == -1) {
+   bkupdir = NULL;
+   return (FALSE);
+   }

if (mkdir(bkupdir, 0700) == -1 && errno != EEXIST) {
free(bkupdir);
@@ -736,7 +737,7 @@ expandtilde(const char *fn)
plen = strlcpy(path, pw->pw_dir, sizeof(path));
if (plen == 0 || path[plen - 1] != '/') {
if (strlcat(path, "/", sizeof(path)) >= sizeof(path)) {
-   dobeep();   
+   dobeep();
ewprintf("Path too long");
return (NULL);
}
Index: pathnames.h
===
RCS file: /cvs/src/usr.bin/mg/pathnames.h,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 pathnames.h
--- pathnames.h 18 Jun 2012 07:14:55 -  1.1
+++ pathnames.h 13 May 2017 04:15:15 -
@@ -6,6 +6,6 @@
  * standard path names
  */

-#define_PATH_MG_DIR"~/.mg.d"
+#define_PATH_MG_DIR"%s/.mg.d"
 #define_PATH_MG_STARTUP"%s/.mg"
 #define_PATH_MG_TERM   "%s/.mg-%s"

Re: Convert explicit_bzero+free to freezero on smtpd(8)

[Gilles Chehade]

On Thu, May 11, 2017 at 11:33:10AM +0100, Ricardo Mestre wrote:
> Hi,
> 
> This converts explicit_bzero+free to freezero on smtpd(8).
> 
> OK?

Sorry i was away from town

I'll have a look at freezero() tomorrow as I missed most of the
discussion about its semantics and I'll ok then

Thanks


> Index: ca.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/ca.c,v
> retrieving revision 1.26
> diff -u -p -u -r1.26 ca.c
> --- ca.c  9 Jan 2017 09:53:23 -   1.26
> +++ ca.c  11 May 2017 10:16:47 -
> @@ -142,8 +142,7 @@ ca_init(void)
>  
>   pki->pki_pkey = pkey;
>  
> - explicit_bzero(pki->pki_key, pki->pki_key_len);
> - free(pki->pki_key);
> + freezero(pki->pki_key, pki->pki_key_len);
>   pki->pki_key = NULL;
>   }
>  }
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/config.c,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 config.c
> --- config.c  1 Sep 2016 10:54:25 -   1.37
> +++ config.c  11 May 2017 10:16:48 -
> @@ -70,12 +70,8 @@ purge_config(uint8_t what)
>   }
>   if (what & PURGE_PKI) {
>   while (dict_poproot(env->sc_pki_dict, (void **))) {
> - explicit_bzero(p->pki_cert, p->pki_cert_len);
> - free(p->pki_cert);
> - if (p->pki_key) {
> - explicit_bzero(p->pki_key, p->pki_key_len);
> - free(p->pki_key);
> - }
> + freezero(p->pki_cert, p->pki_cert_len);
> + freezero(p->pki_key, p->pki_key_len);
>   if (p->pki_pkey)
>   EVP_PKEY_free(p->pki_pkey);
>   free(p);
> @@ -86,14 +82,10 @@ purge_config(uint8_t what)
>   iter_dict = NULL;
>   while (dict_iter(env->sc_pki_dict, _dict, ,
>   (void **))) {
> - explicit_bzero(p->pki_cert, p->pki_cert_len);
> - free(p->pki_cert);
> + freezero(p->pki_cert, p->pki_cert_len);
>   p->pki_cert = NULL;
> - if (p->pki_key) {
> - explicit_bzero(p->pki_key, p->pki_key_len);
> - free(p->pki_key);
> - p->pki_key = NULL;
> - }
> + freezero(p->pki_key, p->pki_key_len);
> + p->pki_key = NULL;
>   if (p->pki_pkey)
>   EVP_PKEY_free(p->pki_pkey);
>   p->pki_pkey = NULL;
> Index: mta_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
> retrieving revision 1.96
> diff -u -p -u -r1.96 mta_session.c
> --- mta_session.c 30 Nov 2016 17:43:32 -  1.96
> +++ mta_session.c 11 May 2017 10:16:50 -
> @@ -341,8 +341,7 @@ mta_session_imsg(struct mproc *p, struct
>   fatal("mta: ssl_mta_init");
>   io_start_tls(s->io, ssl);
>  
> - explicit_bzero(resp_ca_cert->cert, resp_ca_cert->cert_len);
> - free(resp_ca_cert->cert);
> + freezero(resp_ca_cert->cert, resp_ca_cert->cert_len);
>   free(resp_ca_cert);
>   return;
>  
> Index: smtp_session.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
> retrieving revision 1.302
> diff -u -p -u -r1.302 smtp_session.c
> --- smtp_session.c30 Nov 2016 17:43:32 -  1.302
> +++ smtp_session.c11 May 2017 10:16:54 -
> @@ -962,8 +962,7 @@ smtp_session_imsg(struct mproc *p, struc
>   io_set_read(s->io);
>   io_start_tls(s->io, ssl);
>  
> - explicit_bzero(resp_ca_cert->cert, resp_ca_cert->cert_len);
> - free(resp_ca_cert->cert);
> + freezero(resp_ca_cert->cert, resp_ca_cert->cert_len);
>   free(resp_ca_cert);
>   return;
>  

-- 
Gilles Chehade

https://www.poolp.org  @poolpOrg

Re: ipv6 mapped address output

[Alexander Bluhm]

On Mon, May 08, 2017 at 05:09:01PM +0200, Alexander Bluhm wrote:
> Checking for IPv4 mapped addresses is a bit inconsistent in the
> output path.

Here comes the remaining part:

- Do not check for mapped addresses in tcp_usrreq(PRU_CONNECT),
  this is done in in6_pcbconnect().

- Do not check for locally bound mapped addresses in
  in6_pcbconnect(), this is done during bind(2) in in6_pcbaddrisavail().

- Check for mapped addesses in rip6_output() like it is done
  in udp6_output().
- Move the EAFNOSUPPORT error from rip6_usrreq() to rip6_output()
  like it is done for UDP.

- Return EADDRNOTAVAIL if UDP sendto(2) is used with a mapped address.

ok?

bluhm

Index: netinet/tcp_usrreq.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_usrreq.c,v
retrieving revision 1.148
diff -u -p -r1.148 tcp_usrreq.c
--- netinet/tcp_usrreq.c12 May 2017 20:34:29 -  1.148
+++ netinet/tcp_usrreq.c12 May 2017 20:37:33 -
@@ -242,8 +242,7 @@ tcp_usrreq(struct socket *so, int req, s
(nam, struct sockaddr_in6 *)->sin6_addr;
 
if (IN6_IS_ADDR_UNSPECIFIED(addr6) ||
-   IN6_IS_ADDR_MULTICAST(addr6) ||
-   IN6_IS_ADDR_V4MAPPED(addr6)) {
+   IN6_IS_ADDR_MULTICAST(addr6)) {
error = EINVAL;
break;
}
Index: netinet6/in6_pcb.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_pcb.c,v
retrieving revision 1.97
diff -u -p -r1.97 in6_pcb.c
--- netinet6/in6_pcb.c  7 Mar 2017 16:59:40 -   1.97
+++ netinet6/in6_pcb.c  12 May 2017 20:36:12 -
@@ -256,14 +256,9 @@ in6_pcbconnect(struct inpcb *inp, struct
return (EAFNOSUPPORT);
if (sin6->sin6_port == 0)
return (EADDRNOTAVAIL);
-
/* reject IPv4 mapped address, we have no support for it */
if (IN6_IS_ADDR_V4MAPPED(>sin6_addr))
-   return EADDRNOTAVAIL;
-
-   /* sanity check for mapped address case */
-   if (IN6_IS_ADDR_V4MAPPED(>inp_laddr6))
-   return EINVAL;
+   return (EADDRNOTAVAIL);
 
/* protect *sin6 from overwrites */
tmp = *sin6;
Index: netinet6/raw_ip6.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/raw_ip6.c,v
retrieving revision 1.113
diff -u -p -r1.113 raw_ip6.c
--- netinet6/raw_ip6.c  8 May 2017 08:46:39 -   1.113
+++ netinet6/raw_ip6.c  12 May 2017 20:37:22 -
@@ -343,7 +343,6 @@ rip6_output(struct mbuf *m, struct socke
priv = 0;
if ((so->so_state & SS_PRIV) != 0)
priv = 1;
-   dst = (dstaddr)->sin6_addr;
if (control) {
if ((error = ip6_setpktopts(control, ,
in6p->inp_outputopts6,
@@ -353,6 +352,16 @@ rip6_output(struct mbuf *m, struct socke
} else
optp = in6p->inp_outputopts6;
 
+   if (dstaddr->sa_family != AF_INET6) {
+   error = EAFNOSUPPORT;
+   goto bad;
+   }
+   dst = (dstaddr)->sin6_addr;
+   if (IN6_IS_ADDR_V4MAPPED(dst)) {
+   error = EADDRNOTAVAIL;
+   goto bad;
+   }
+
/*
 * For an ICMPv6 packet, we should know its type and code
 * to update statistics.
@@ -691,11 +700,6 @@ rip6_usrreq(struct socket *so, int req, 
 
tmp = *mtod(nam, struct sockaddr_in6 *);
dst = 
-
-   if (dst->sin6_family != AF_INET6) {
-   error = EAFNOSUPPORT;
-   break;
-   }
}
error = rip6_output(m, so, sin6tosa(dst), control);
m = NULL;
Index: netinet6/udp6_output.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/udp6_output.c,v
retrieving revision 1.53
diff -u -p -r1.53 udp6_output.c
--- netinet6/udp6_output.c  19 Dec 2016 15:47:19 -  1.53
+++ netinet6/udp6_output.c  12 May 2017 20:36:12 -
@@ -132,7 +132,7 @@ udp6_output(struct inpcb *in6p, struct m
goto release;
}
if (IN6_IS_ADDR_V4MAPPED(>sin6_addr)) {
-   error = EINVAL;
+   error = EADDRNOTAVAIL;
goto release;
}
 

Pf manpage update for FQ-CoDel

[Mike Belopuhov]

I've tried very hard to make it concise and avoided any references to
underlying algorithms.

OK?

---
 share/man/man5/pf.conf.5 | 47 ---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git share/man/man5/pf.conf.5 share/man/man5/pf.conf.5
index e0e8e897768..5c886c0cb3a 100644
--- share/man/man5/pf.conf.5
+++ share/man/man5/pf.conf.5
@@ -46,11 +46,11 @@ This is an overview of the sections in this manual page:
 .It Sx PACKET FILTERING
 including network address translation (NAT).
 .It Sx OPTIONS
 globally tune the behaviour of the packet filtering engine.
 .It Sx QUEUEING
-provides rule-based bandwidth control.
+provides rule-based bandwidth and traffic control.
 .It Sx TABLES
 provide a method for dealing with large numbers of addresses.
 .It Sx ANCHORS
 are containers for rules and tables.
 .It Sx STATEFUL FILTERING
@@ -1562,10 +1562,48 @@ pass out on em0 inet proto tcp from $employeehosts to 
any port 80 \e
 pass out on em0 inet proto tcp from any to any port 22 \e
   set queue(ssh_bulk, ssh_interactive)
 pass out on em0 inet proto tcp from any to any port 25 \e
   set queue mail
 .Ed
+.Pp
+Additionally to the bandwidth management a fair traffic sharing option
+is provided via a flow queue.
+When packets are classified by the stateful inspection engine, a flow
+identifier is assigned to all packets belonging to this state.
+A flow queue splits the outgoing traffic according to these identifiers
+into individual queues and services them in a way that provides equal
+opportunities for all connections to progress.
+.Pp
+Configuration of a flow queue is similar to a regular one, however flow
+queues don't build a hierarchy and thus only one flow queue may be
+specified per interface.
+.Pp
+The core parameter of a flow queue is a number of expected simultaneous
+connections, or
+.Cm flows ,
+bounded by the resolution of the flow identifier (the current
+implementation is able classify traffic into 32767 distinct flows).
+Thus minimal flow queue configuration is:
+.Bd -literal -offset indent
+queue fq on em0 flows 1024
+.Ed
+.Pp
+Another value affecting the flow queue is a quantum of service.
+The lower the
+.Cm quantum
+the more advantage is given to streams of small packets at the expense
+of bulk transfers.
+The default value for
+.Cm quantum
+is selected based on the configured Maximum Transmission Unit (MTU)
+of the specified interface.
+.Pp
+An additional configuration option
+.Cm qlimit
+sets the queue depth shared among all flows.
+The default is 1024.
+.El
 .Sh TABLES
 Tables are named structures which can hold a collection of addresses and
 networks.
 Lookups against tables in
 .Xr pf 4
@@ -2693,13 +2731,16 @@ anchor-rule= "anchor" [ string ] [ ( "in" | "out" ) 
] [ "on" ifspec ]
 anchor-close   = "}"
 
 load-anchor= "load anchor" string "from" filename
 
 queueopts-list = queueopts-list queueopts | queueopts
-queueopts  = [ "bandwidth" bandwidth ] | [ "min" bandwidth ] |
+queueopts  = ([ "bandwidth" bandwidth ] | [ "min" bandwidth ] |
  [ "max" bandwidth ] | [ "parent" string ] |
- [ "default" ] | [ "qlimit" number ]
+ [ "default" ]) |
+ ([ "flows" number ] | [ "quantum" number ]) |
+ [ "qlimit" number ]
+
 bandwidth  = bandwidth-spec [ "burst" bandwidth-spec "for" number "ms" ]
 bandwidth-spec = number ( "" | "K" | "M" | "G" )
 
 action = "pass" | "match" | "block" [ return ]
 return = "drop" | "return" |
-- 
2.12.2

Re: Pf interface for FQ-CoDel

[Mike Belopuhov]

On Fri, Apr 28, 2017 at 19:58 +0200, Mike Belopuhov wrote:
> This is the last bit required in order to actually be able to
> use FQ-CoDel.  It might require some polish, but I think there's
> nothing exceptionally ugly barring the statistics interface.
> 
> fqcodel_stats is constructed to have a few fields "compatible"
> with the hfsc_class_stats so that byte and packet counters can
> be extracted in the exactly the same way.
> 
> The man page will follow.
> 
> OK?
> 

Here's an updated version with some additional polish.

I've already got FQ-CoDel bits in so they're no longer part of this
diff and I've also removed "interval" and "target" keywords for now:
they need to be properly documented and at the moment I can't come up
with a comprehensible description so to be honest with myself I'm
withholding those bits until later.

OK?

---
 sbin/pfctl/parse.y| 64 ---
 sbin/pfctl/pfctl.c|  6 ++---
 sbin/pfctl/pfctl_parser.c | 26 ---
 sbin/pfctl/pfctl_queue.c  | 53 +--
 sys/conf/files|  1 +
 sys/net/pf_ioctl.c| 17 ++---
 sys/net/pfvar.h   | 11 
 7 files changed, 146 insertions(+), 32 deletions(-)

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 4f49b102609..736d418071f 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -34,11 +34,10 @@
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #include 
 #include 
 #include 
@@ -304,19 +303,29 @@ struct node_sc {
struct node_queue_bwm1;
u_int   d;
struct node_queue_bwm2;
 };
 
+struct node_fq {
+   u_int   flows;
+   u_int   quantum;
+   u_int   target;
+   u_int   interval;
+};
+
 struct queue_opts {
int  marker;
 #defineQOM_BWSPEC  0x01
 #defineQOM_PARENT  0x02
 #defineQOM_DEFAULT 0x04
 #defineQOM_QLIMIT  0x08
+#defineQOM_FLOWS   0x10
+#defineQOM_QUANTUM 0x20
struct node_sc   realtime;
struct node_sc   linkshare;
struct node_sc   upperlimit;
+   struct node_fq   flowqueue;
char*parent;
int  flags;
u_intqlimit;
 } queue_opts;
 
@@ -457,11 +466,11 @@ int   parseport(char *, struct range *r, int);
 %token REASSEMBLE ANCHOR
 %token SET OPTIMIZATION TIMEOUT LIMIT LOGINTERFACE BLOCKPOLICY RANDOMID
 %token SYNPROXY FINGERPRINTS NOSYNC DEBUG SKIP HOSTID
 %token ANTISPOOF FOR INCLUDE MATCHES
 %token BITMASK RANDOM SOURCEHASH ROUNDROBIN LEASTSTATES STATICPORT PROBABILITY
-%token WEIGHT BANDWIDTH
+%token WEIGHT BANDWIDTH FLOWS QUANTUM
 %token QUEUE PRIORITY QLIMIT RTABLE RDOMAIN MINIMUM BURST PARENT
 %token LOAD RULESET_OPTIMIZATION RTABLE RDOMAIN PRIO ONCE DEFAULT
 %token STICKYADDRESS MAXSRCSTATES MAXSRCNODES SOURCETRACK GLOBAL RULE
 %token MAXSRCCONN MAXSRCCONNRATE OVERLOAD FLUSH SLOPPY PFLOW
 %token TAGGED TAG IFBOUND FLOATING STATEPOLICY STATEDEFAULTS ROUTE
@@ -1317,10 +1326,15 @@ queue_opts_l: queue_opts_l queue_opt
 queue_opt  : BANDWIDTH scspec optscs   {
if (queue_opts.marker & QOM_BWSPEC) {
yyerror("bandwidth cannot be respecified");
YYERROR;
}
+   if (queue_opts.marker & QOM_FLOWS) {
+   yyerror("bandwidth cannot be specified for "
+   "a flow queue");
+   YYERROR;
+   }
queue_opts.marker |= QOM_BWSPEC;
queue_opts.linkshare = $2;
queue_opts.realtime= $3.realtime;
queue_opts.upperlimit = $3.upperlimit;
}
@@ -1336,13 +1350,13 @@ queue_opt   : BANDWIDTH scspec optscs   
{
if (queue_opts.marker & QOM_DEFAULT) {
yyerror("default cannot be respecified");
YYERROR;
}
queue_opts.marker |= QOM_DEFAULT;
-   queue_opts.flags |= HFSC_DEFAULTCLASS;
+   queue_opts.flags |= PFQS_DEFAULT;
}
-   | QLIMIT NUMBER {
+   | QLIMIT NUMBER {
if (queue_opts.marker & QOM_QLIMIT) {
yyerror("qlimit cannot be respecified");
YYERROR;
}
if ($2 < 0 || $2 > 65535) {
@@ -1350,10 +1364,41 @@ queue_opt   : BANDWIDTH scspec optscs   
{
YYERROR;
}
   

Re: ipv6 mapped address output

[Mike Belopuhov]

On 12 May 2017 at 18:13, Alexander Bluhm  wrote:

> On Mon, May 08, 2017 at 05:09:01PM +0200, Alexander Bluhm wrote:
> > Checking for IPv4 mapped addresses is a bit inconsistent in the
> > output path.
>
> I should split the diff to make review easier.
>
> - Use the common switch(af) construct for af specific code in
>   tcp_usrreq(PRU_CONNECT).
> - Do not access sockaddr_in before checking the address family.
> - Add a EAFNOSUPPORT default case.
>
> ok?
>
> bluhm
>
>
Looks good to me. OK mikeb

Re: ipsec panic early

[Mike Belopuhov]

On 12 May 2017 at 18:08, Alexander Bluhm  wrote:

> On Fri, May 12, 2017 at 05:56:09PM +0200, Mike Belopuhov wrote:
> > No, there's a check just above...
>
> And without the panic?  Remove duplicate code, remove if (proto == 0)
> that cannot happen.
>
> bluhm
>
>
Sure.

Re: ipv6 mapped address output

[Alexander Bluhm]

On Mon, May 08, 2017 at 05:09:01PM +0200, Alexander Bluhm wrote:
> Checking for IPv4 mapped addresses is a bit inconsistent in the
> output path.

I should split the diff to make review easier.

- Use the common switch(af) construct for af specific code in
  tcp_usrreq(PRU_CONNECT).
- Do not access sockaddr_in before checking the address family.
- Add a EAFNOSUPPORT default case.

ok?

bluhm

Index: netinet/tcp_usrreq.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_usrreq.c,v
retrieving revision 1.147
diff -u -p -r1.147 tcp_usrreq.c
--- netinet/tcp_usrreq.c5 Apr 2017 13:35:18 -   1.147
+++ netinet/tcp_usrreq.c12 May 2017 16:09:16 -
@@ -127,7 +127,6 @@ int
 tcp_usrreq(struct socket *so, int req, struct mbuf *m, struct mbuf *nam,
 struct mbuf *control, struct proc *p)
 {
-   struct sockaddr_in *sin;
struct inpcb *inp;
struct tcpcb *tp = NULL;
int error = 0;
@@ -221,33 +220,41 @@ tcp_usrreq(struct socket *so, int req, s
 * Send initial segment on connection.
 */
case PRU_CONNECT:
-   sin = mtod(nam, struct sockaddr_in *);
-
-#ifdef INET6
-   if (sin->sin_family == AF_INET6) {
-   struct in6_addr *in6_addr = (nam,
-   struct sockaddr_in6 *)->sin6_addr;
-
-   if (IN6_IS_ADDR_UNSPECIFIED(in6_addr) ||
-   IN6_IS_ADDR_MULTICAST(in6_addr) ||
-   IN6_IS_ADDR_V4MAPPED(in6_addr)) {
+   switch (mtod(nam, struct sockaddr *)->sa_family) {
+   case AF_INET: {
+   struct in_addr *addr =
+   (nam, struct sockaddr_in *)->sin_addr;
+
+   if ((addr->s_addr == INADDR_ANY) ||
+   (addr->s_addr == INADDR_BROADCAST) ||
+   IN_MULTICAST(addr->s_addr) ||
+   in_broadcast(*addr, inp->inp_rtableid)) {
error = EINVAL;
break;
}
 
-   error = in6_pcbconnect(inp, nam);
-   } else if (sin->sin_family == AF_INET)
-#endif /* INET6 */
-   {
-   if ((sin->sin_addr.s_addr == INADDR_ANY) ||
-   (sin->sin_addr.s_addr == INADDR_BROADCAST) ||
-   IN_MULTICAST(sin->sin_addr.s_addr) ||
-   in_broadcast(sin->sin_addr, inp->inp_rtableid)) {
+   error = in_pcbconnect(inp, nam);
+   break;
+   }
+#ifdef INET6
+   case AF_INET6: {
+   struct in6_addr *addr6 =
+   (nam, struct sockaddr_in6 *)->sin6_addr;
+
+   if (IN6_IS_ADDR_UNSPECIFIED(addr6) ||
+   IN6_IS_ADDR_MULTICAST(addr6) ||
+   IN6_IS_ADDR_V4MAPPED(addr6)) {
error = EINVAL;
break;
}
 
-   error = in_pcbconnect(inp, nam);
+   error = in6_pcbconnect(inp, nam);
+   break;
+   }
+#endif /* INET6 */
+   default:
+   error = EAFNOSUPPORT;
+   break;
}
 
if (error)

Re: ipsec panic early

[Alexander Bluhm]

On Fri, May 12, 2017 at 05:56:09PM +0200, Mike Belopuhov wrote:
> No, there's a check just above...

And without the panic?  Remove duplicate code, remove if (proto == 0)
that cannot happen.

bluhm

Index: net/if_bridge.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.294
diff -u -p -r1.294 if_bridge.c
--- net/if_bridge.c 5 Feb 2017 16:04:14 -   1.294
+++ net/if_bridge.c 12 May 2017 16:04:49 -
@@ -1425,17 +1425,6 @@ bridge_ipsec(struct bridge_softc *sc, st
sizeof(struct in_addr),
(caddr_t)_addr);
 
-   if (ip->ip_p == IPPROTO_ESP)
-   m_copydata(m, hlen, sizeof(u_int32_t),
-   (caddr_t));
-   else if (ip->ip_p == IPPROTO_AH)
-   m_copydata(m, hlen + sizeof(u_int32_t),
-   sizeof(u_int32_t), (caddr_t));
-   else if (ip->ip_p == IPPROTO_IPCOMP) {
-   m_copydata(m, hlen + sizeof(u_int16_t),
-   sizeof(u_int16_t), (caddr_t));
-   spi = ntohl(htons(cpi));
-   }
break;
 #ifdef INET6
case AF_INET6:
@@ -1459,25 +1448,26 @@ bridge_ipsec(struct bridge_softc *sc, st
sizeof(struct in6_addr),
(caddr_t)_addr);
 
-   if (proto == IPPROTO_ESP)
-   m_copydata(m, hlen, sizeof(u_int32_t),
-   (caddr_t));
-   else if (proto == IPPROTO_AH)
-   m_copydata(m, hlen + sizeof(u_int32_t),
-   sizeof(u_int32_t), (caddr_t));
-   else if (proto == IPPROTO_IPCOMP) {
-   m_copydata(m, hlen + sizeof(u_int16_t),
-   sizeof(u_int16_t), (caddr_t));
-   spi = ntohl(htons(cpi));
-   }
break;
 #endif /* INET6 */
default:
return (0);
}
 
-   if (proto == 0)
-   goto skiplookup;
+   switch (proto) {
+   case IPPROTO_ESP:
+   m_copydata(m, hlen, sizeof(u_int32_t), (caddr_t));
+   break;
+   case IPPROTO_AH:
+   m_copydata(m, hlen + sizeof(u_int32_t),
+   sizeof(u_int32_t), (caddr_t));
+   break;
+   case IPPROTO_IPCOMP:
+   m_copydata(m, hlen + sizeof(u_int16_t),
+   sizeof(u_int16_t), (caddr_t));
+   spi = ntohl(htons(cpi));
+   break;
+   }
 
splsoftassert(IPL_SOFTNET);
 

multipath / route priority support for ospf6d

[Florian Riehm]

Hi,

our QA reports issues with the ospf6d since the kernel uses more multipath
routes.
It exits after certain topology changes with:
rde_send_change_kroute: no valid nexthop found

Since the kernel uses more multipath routes, the lack of multipath support in
ospf6d became a problem.

The attached patch ports the multipath support from ospfd to ospf6d.
It bases on the following ospfd commits:
cvs diff -D "2007-09-24" -D "2007-09-26"
cvs diff -r1.65 -r1.67 kroute.c

A similar patch was suggested by Manues Guesdon a couple of years ago.

This patch doesn't fix all problems I am seeing, but it improves the situation
a lot. A second patch porting priority support will follow and fix further
issues. I decided to split my fixes into two parts to make review easier.

Thanks & Regards

Florian

Index: kroute.c
===
RCS file: /openbsd/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.50
diff -u -p -r1.50 kroute.c
--- kroute.c27 Dec 2016 17:18:56 -  1.50
+++ kroute.c11 May 2017 20:48:49 -
@@ -59,6 +59,8 @@ void  kr_redist_remove(struct kroute_node
 intkr_redist_eval(struct kroute *, struct kroute *);
 void   kr_redistribute(struct kroute_node *);
 intkroute_compare(struct kroute_node *, struct kroute_node *);
+intkr_change_fib(struct kroute_node *, struct kroute *, int, int);
+intkr_delete_fib(struct kroute_node *);
 
 struct kroute_node	*kroute_find(const struct in6_addr *, u_int8_t);

 struct kroute_node *kroute_matchgw(struct kroute_node *,
@@ -141,18 +143,105 @@ kr_init(int fs)
 }
 
 int

-kr_change(struct kroute *kroute)
+kr_change_fib(struct kroute_node *kr, struct kroute *kroute, int krcount,
+int action)
+{
+   int  i;
+   struct kroute_node  *kn, *nkn;
+
+   if (action == RTM_ADD) {
+   /*
+* First remove all stale multipath routes.
+* This step must be skipped when the action is RTM_CHANGE
+* because it is already a single path route that will be
+* changed.
+*/
+   for (kn = kr; kn != NULL; kn = nkn) {
+   for (i = 0; i < krcount; i++) {
+   if (kn->r.scope == kroute[i].scope &&
+   IN6_ARE_ADDR_EQUAL(>r.nexthop,
+   [i].nexthop))
+   break;
+   }
+   nkn = kn->next;
+   if (i == krcount) {
+   /* stale route */
+   if (kr_delete_fib(kn) == -1)
+   log_warnx("kr_delete_fib failed");
+   /*
+* if head element was removed we need to adjust
+* the head
+*/
+   if (kr == kn)
+   kr = nkn;
+   }
+   }
+   }
+
+   /*
+* now add or change the route
+*/
+   for (i = 0; i < krcount; i++) {
+   /* nexthop ::1 -> ignore silently */
+   if (IN6_IS_ADDR_LOOPBACK([i].nexthop))
+   continue;
+
+   if (action == RTM_ADD && kr) {
+   for (kn = kr; kn != NULL; kn = kn->next) {
+   if (kn->r.scope == kroute[i].scope &&
+   IN6_ARE_ADDR_EQUAL(>r.nexthop,
+   [i].nexthop))
+   break;
+   }
+
+   if (kn != NULL)
+   /* nexthop already present, skip it */
+   continue;
+   } else
+   /* modify first entry */
+   kn = kr;
+
+   /* send update */
+   if (send_rtmsg(kr_state.fd, action, [i]) == -1)
+   return (-1);
+
+   /* create new entry unless we are changing the first entry */
+   if (action == RTM_ADD)
+   if ((kn = calloc(1, sizeof(*kn))) == NULL)
+   fatal(NULL);
+
+   kn->r.prefix = kroute[i].prefix;
+   kn->r.prefixlen = kroute[i].prefixlen;
+   kn->r.nexthop = kroute[i].nexthop;
+   kn->r.scope = kroute[i].scope;
+   kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED;
+   kn->r.ext_tag = kroute[i].ext_tag;
+   rtlabel_unref(kn->r.rtlabel);/* for RTM_CHANGE */
+   kn->r.rtlabel = kroute[i].rtlabel;
+
+   if (action == RTM_ADD)
+   if (kroute_insert(kn) == -1) {
+   log_debug("kr_update_fib: cannot insert %s",
+ 

Re: ipsec panic early

[Mike Belopuhov]

On 12 May 2017 at 17:28, Alexander Bluhm  wrote:

> On Fri, May 12, 2017 at 01:53:12PM +0200, Alexander Bluhm wrote:
> > In bridge_ipsec() tdb comes from
> > gettdb() called with proto.  There we goto skiplookup if proto !=
> > IPPROTO_ESP && proto != IPPROTO_AH && proto != IPPROTO_IPCOMP.
>
> While looking at this, I saw the same code in the IPv4 and IPv6
> case.  And we could put the panic there, too.
>
> ok?
>
> bluhm
>
>
No, there's a check just above...

Re: ipsec panic early

[Alexander Bluhm]

On Fri, May 12, 2017 at 01:53:12PM +0200, Alexander Bluhm wrote:
> In bridge_ipsec() tdb comes from
> gettdb() called with proto.  There we goto skiplookup if proto !=
> IPPROTO_ESP && proto != IPPROTO_AH && proto != IPPROTO_IPCOMP.

While looking at this, I saw the same code in the IPv4 and IPv6
case.  And we could put the panic there, too.

ok?

bluhm

Index: net/if_bridge.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.294
diff -u -p -r1.294 if_bridge.c
--- net/if_bridge.c 5 Feb 2017 16:04:14 -   1.294
+++ net/if_bridge.c 12 May 2017 14:51:35 -
@@ -1425,17 +1425,6 @@ bridge_ipsec(struct bridge_softc *sc, st
sizeof(struct in_addr),
(caddr_t)_addr);
 
-   if (ip->ip_p == IPPROTO_ESP)
-   m_copydata(m, hlen, sizeof(u_int32_t),
-   (caddr_t));
-   else if (ip->ip_p == IPPROTO_AH)
-   m_copydata(m, hlen + sizeof(u_int32_t),
-   sizeof(u_int32_t), (caddr_t));
-   else if (ip->ip_p == IPPROTO_IPCOMP) {
-   m_copydata(m, hlen + sizeof(u_int16_t),
-   sizeof(u_int16_t), (caddr_t));
-   spi = ntohl(htons(cpi));
-   }
break;
 #ifdef INET6
case AF_INET6:
@@ -1459,25 +1448,29 @@ bridge_ipsec(struct bridge_softc *sc, st
sizeof(struct in6_addr),
(caddr_t)_addr);
 
-   if (proto == IPPROTO_ESP)
-   m_copydata(m, hlen, sizeof(u_int32_t),
-   (caddr_t));
-   else if (proto == IPPROTO_AH)
-   m_copydata(m, hlen + sizeof(u_int32_t),
-   sizeof(u_int32_t), (caddr_t));
-   else if (proto == IPPROTO_IPCOMP) {
-   m_copydata(m, hlen + sizeof(u_int16_t),
-   sizeof(u_int16_t), (caddr_t));
-   spi = ntohl(htons(cpi));
-   }
break;
 #endif /* INET6 */
default:
return (0);
}
 
-   if (proto == 0)
-   goto skiplookup;
+   switch (proto) {
+   case IPPROTO_ESP:
+   m_copydata(m, hlen, sizeof(u_int32_t), (caddr_t));
+   break;
+   case IPPROTO_AH:
+   m_copydata(m, hlen + sizeof(u_int32_t),
+   sizeof(u_int32_t), (caddr_t));
+   break;
+   case IPPROTO_IPCOMP:
+   m_copydata(m, hlen + sizeof(u_int16_t),
+   sizeof(u_int16_t), (caddr_t));
+   spi = ntohl(htons(cpi));
+   break;
+   default:
+   panic("%s: unknown/unsupported security protocol %d",
+   __func__, proto);
+   }
 
splsoftassert(IPL_SOFTNET);
 

Re: IPsec IPv4 local delivery

[Mike Belopuhov]

On 12 May 2017 at 15:29, Alexander Bluhm  wrote:

> Hi,
>
> IPsec packets are passed through ip_input() a second time after
> they have been decrypted.  That means that all the IP header fields
> are checked twice.  Also fragment reassembly is tried twice.
>
> In pf incoming packets in tunnel mode appear twice on the enc0
> interface.  Once as IP-in-IP and once as the inner packet.  In the
> outgoing path we only see the inner packet.  Asymmetry is bad for
> stateful filtering.
>
> IPv6 shows that IPsec works without that.  After decrypting we can
> continue with local delivery.  If we are in tunnel mode, the IP-in-IP
> protocol functions do what we want.  In transport mode only pf_test()
> has to be called for the enc0 device.
>
> Introducing ip_local() means less needless processing and cleaner
> pf behavior.
>
> ok?
>
> bluhm
>
>
Nice. OK mikeb

Re: Atomic copyin(9)/copyout(9) for amd64

[Visa Hankala]

On Mon, May 01, 2017 at 06:02:24PM +0200, Mark Kettenis wrote:
> The futex(2) syscall needs to be able to atomically copy the futex in
> and out of userland.  The current implementation uses copyin(9) and
> copyout(9) for that.  The futex is a 32-bit integer, and currently our
> copyin(9) and copyout(9) don't guarantee an atomic 32-bit access.
> Previously mpi@ and I discussed implementing new interfaces that do
> guarantee the required atomicity.  However, it oocurred to me that we
> could simply change our copyin implementations such that they
> guarantee atomicity of a properly aligned 32-bit copyin and copyout.
> 
> The i386 version of these calls uses "rep movsl", which means it is
> already atomic.  At least that is how I interpret 8.2.4 in Volume 3A
> of the Intel SDM.  The diff below makes the amd64 version safe as
> well.  This does introduce a few additional instructions in the loop.
> Apparently modern Intel CPUs optimize the string loops.  If we can
> rely on the hardware to turn 32-bit moves into 64-bit moves, we could
> simplify the code by using "rep movsl" instead of "rep movsq".

I submit to this approach. OK visa@

> Index: arch/amd64/amd64/copy.S
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/copy.S,v
> retrieving revision 1.7
> diff -u -p -r1.7 copy.S
> --- arch/amd64/amd64/copy.S   25 Apr 2015 21:31:24 -  1.7
> +++ arch/amd64/amd64/copy.S   1 May 2017 15:32:17 -
> @@ -138,7 +138,12 @@ ENTRY(copyout)
>   rep
>   movsq
>   movb%al,%cl
> - andb$7,%cl
> + shrb$2,%cl
> + andb$1,%cl
> + rep
> + movsl
> + movb%al,%cl
> + andb$3,%cl
>   rep
>   movsb
>   SMAP_CLAC
> @@ -168,7 +173,12 @@ ENTRY(copyin)
>   rep
>   movsq
>   movb%al,%cl
> - andb$7,%cl
> + shrb$2,%cl
> + andb$1,%cl
> + rep
> + movsl
> + movb%al,%cl
> + andb$3,%cl
>   rep
>   movsb
>  
> 

IPsec IPv4 local delivery

[Alexander Bluhm]

Hi,

IPsec packets are passed through ip_input() a second time after
they have been decrypted.  That means that all the IP header fields
are checked twice.  Also fragment reassembly is tried twice.

In pf incoming packets in tunnel mode appear twice on the enc0
interface.  Once as IP-in-IP and once as the inner packet.  In the
outgoing path we only see the inner packet.  Asymmetry is bad for
stateful filtering.

IPv6 shows that IPsec works without that.  After decrypting we can
continue with local delivery.  If we are in tunnel mode, the IP-in-IP
protocol functions do what we want.  In transport mode only pf_test()
has to be called for the enc0 device.

Introducing ip_local() means less needless processing and cleaner
pf behavior.

ok?

bluhm

Index: netinet/ip_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.299
diff -u -p -r1.299 ip_input.c
--- netinet/ip_input.c  11 May 2017 11:36:20 -  1.299
+++ netinet/ip_input.c  12 May 2017 12:34:42 -
@@ -483,9 +483,6 @@ ip_ours(struct mbuf *m)
 
hlen = ip->ip_hl << 2;
 
-   /* pf might have modified stuff, might have to chksum */
-   in_proto_cksum_out(m, NULL);
-
/*
 * If offset or IP_MF are set, must reassemble.
 * Otherwise, nothing need be done.
@@ -570,11 +567,26 @@ found:
ip_freef(fp);
}
 
+   ip_local(m, hlen, ip->ip_p);
+   return;
+bad:
+   m_freem(m);
+}
+
+void
+ip_local(struct mbuf *m, int off, int nxt)
+{
+   KERNEL_ASSERT_LOCKED();
+
+   /* pf might have modified stuff, might have to chksum */
+   in_proto_cksum_out(m, NULL);
+
 #ifdef IPSEC
if (ipsec_in_use) {
-   if (ip_input_ipsec_ours_check(m, hlen) != 0) {
+   if (ip_input_ipsec_ours_check(m, off) != 0) {
ipstat_inc(ips_cantforward);
-   goto bad;
+   m_freem(m);
+   return;
}
}
/* Otherwise, just fall through and deliver the packet */
@@ -584,10 +596,7 @@ found:
 * Switch out to protocol's input routine.
 */
ipstat_inc(ips_delivered);
-   (*inetsw[ip_protox[ip->ip_p]].pr_input)(, , ip->ip_p, AF_INET);
-   return;
-bad:
-   m_freem(m);
+   (*inetsw[ip_protox[nxt]].pr_input)(, , nxt, AF_INET);
 }
 
 int
Index: netinet/ip_var.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_var.h,v
retrieving revision 1.71
diff -u -p -r1.71 ip_var.h
--- netinet/ip_var.h14 Apr 2017 20:46:31 -  1.71
+++ netinet/ip_var.h12 May 2017 12:34:42 -
@@ -249,6 +249,7 @@ void ip_savecontrol(struct inpcb *, str
struct mbuf *);
 voidipintr(void);
 voidipv4_input(struct mbuf *);
+voidip_local(struct mbuf *, int, int);
 voidip_forward(struct mbuf *, struct ifnet *, struct rtentry *, int);
 int rip_ctloutput(int, struct socket *, int, int, struct mbuf *);
 voidrip_init(void);
Index: netinet/ipsec_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v
retrieving revision 1.149
diff -u -p -r1.149 ipsec_input.c
--- netinet/ipsec_input.c   11 May 2017 12:14:43 -  1.149
+++ netinet/ipsec_input.c   12 May 2017 12:51:51 -
@@ -579,44 +579,38 @@ ipsec_common_input_cb(struct mbuf *m, st
return;
}
 
+#if NPF > 0
+   /*
+* The ip_local() shortcut avoids running through ip_input() with the
+* same IP header twice.  Packets in transport mode have to be be
+* passed to pf explicitly.  In tunnel mode the inner IP header will
+* run through ip_input() and pf anyway.
+*/
+   if ((tdbp->tdb_flags & TDBF_TUNNELING) == 0) {
+   struct ifnet *ifp;
+
+   /* This is the enc0 interface unless for ipcomp. */
+   if ((ifp = if_get(m->m_pkthdr.ph_ifidx)) == NULL) {
+   m_freem(m);
+   return;
+   }
+   if (pf_test(af, PF_IN, ifp, ) != PF_PASS) {
+   if_put(ifp);
+   m_freem(m);
+   return;
+   }
+   if_put(ifp);
+   if (m == NULL)
+   return;
+   }
+#endif
/* Call the appropriate IPsec transform callback. */
switch (af) {
case AF_INET:
-   if (niq_enqueue(, m) != 0) {
-   DPRINTF(("ipsec_common_input_cb(): dropped packet "
-   "because of full IP queue\n"));
-   IPSEC_ISTAT(espstat.esps_qfull, ahstat.ahs_qfull,
-   ipcompstat.ipcomps_qfull);
-   }
+   ip_local(m, skip, prot);

Re: ipsec panic early

[Claudio Jeker]

On Fri, May 12, 2017 at 01:53:12PM +0200, Alexander Bluhm wrote:
> On Fri, May 12, 2017 at 07:30:28AM +0100, Tom Cosgrove wrote:
> > >>> Alexander Bluhm 11-May-17 23:25 >>>
> > > Instead of printing a debug message at the end, panic early if the
> > > IPsec security protocol is unknown.
> > 
> > Is this before or after we have decrypted and checked MAC?  TBH, even if
> > it's after, would this mean a compromise of a remote machine could be used
> > to crash the running system's kernel?
> 
> Of course it is risky to put a panic() into the IP input path.  But
> to find bugs and to understand the code it is better than a DPRINTF()
> nobody has cared about for 15 years.  I think the default case with
> the panic() can only be triggered by programming errors.
> 
> The variable sproto is passed as parmeter to ipsec_common_input().
> It gets called by ah4_input(), esp4_input(), ipcomp4_input(),
> ah6_input(), esp6_input(), ipcomp6_input() where sproto comes from
> the proto parameter.  These are only called by the pr_input callbacks
> where the protocol is one of IPPROTO_ESP, IPPROTO_AH, IPPROTO_IPCOMP.
> 
> In ipsec_common_input_cb() sproto comes from tdbp->tdb_sproto.  It
> is set in reserve_spi() which gets its sproto from sa1->tdb_sproto
> in pfkeyv2_send().  This field is set by pfkeyv2_get_proto_alg(),
> possible values are IPPROTO_AH, IPPROTO_ESP, IPPROTO_IPIP,
> IPPROTO_IPCOMP, IPPROTO_TCP.
> 
> So why is ipsec_common_input_cb() not called with IPPROTO_IPIP or
> IPPROTO_TCP?
> 
> ipsec_common_input_cb() is called by ah_input_cb(), esp_input_cb(),
> ipcomp_input_cb() with the tdb coming from gettdb().  gettdb() is
> called with tc->tc_proto and only returns a tdb with matching
> tdb_sproto.  The input callbacks are registered in ah_input(),
> esp_input(), ipcomp_input(), there tc->tc_proto is set to tdb->tdb_sproto
> or IPPROTO_IPCOMP.  tdb is passed to ah_input(), esp_input() as
> parameter, they are called via the xf_input pointer.  This is called
> from ipsec_common_input() and bridge_ipsec().  In ipsec_common_input()
> tdbp comes from gettdb() called with sproto passed as parameter.
> This has been discussed before.  In bridge_ipsec() tdb comes from
> gettdb() called with proto.  There we goto skiplookup if proto !=
> IPPROTO_ESP && proto != IPPROTO_AH && proto != IPPROTO_IPCOMP.
> 
> The question is, do we trust this analysis?
> 

I came to a similar conclusion and I think now it is the right time to
take this risk. We may decide to make this a non fatal error return later
on.

OK claudio@
-- 
:wq Claudio

Re: ipsec panic early

[Alexander Bluhm]

On Fri, May 12, 2017 at 07:30:28AM +0100, Tom Cosgrove wrote:
> >>> Alexander Bluhm 11-May-17 23:25 >>>
> > Instead of printing a debug message at the end, panic early if the
> > IPsec security protocol is unknown.
> 
> Is this before or after we have decrypted and checked MAC?  TBH, even if
> it's after, would this mean a compromise of a remote machine could be used
> to crash the running system's kernel?

Of course it is risky to put a panic() into the IP input path.  But
to find bugs and to understand the code it is better than a DPRINTF()
nobody has cared about for 15 years.  I think the default case with
the panic() can only be triggered by programming errors.

The variable sproto is passed as parmeter to ipsec_common_input().
It gets called by ah4_input(), esp4_input(), ipcomp4_input(),
ah6_input(), esp6_input(), ipcomp6_input() where sproto comes from
the proto parameter.  These are only called by the pr_input callbacks
where the protocol is one of IPPROTO_ESP, IPPROTO_AH, IPPROTO_IPCOMP.

In ipsec_common_input_cb() sproto comes from tdbp->tdb_sproto.  It
is set in reserve_spi() which gets its sproto from sa1->tdb_sproto
in pfkeyv2_send().  This field is set by pfkeyv2_get_proto_alg(),
possible values are IPPROTO_AH, IPPROTO_ESP, IPPROTO_IPIP,
IPPROTO_IPCOMP, IPPROTO_TCP.

So why is ipsec_common_input_cb() not called with IPPROTO_IPIP or
IPPROTO_TCP?

ipsec_common_input_cb() is called by ah_input_cb(), esp_input_cb(),
ipcomp_input_cb() with the tdb coming from gettdb().  gettdb() is
called with tc->tc_proto and only returns a tdb with matching
tdb_sproto.  The input callbacks are registered in ah_input(),
esp_input(), ipcomp_input(), there tc->tc_proto is set to tdb->tdb_sproto
or IPPROTO_IPCOMP.  tdb is passed to ah_input(), esp_input() as
parameter, they are called via the xf_input pointer.  This is called
from ipsec_common_input() and bridge_ipsec().  In ipsec_common_input()
tdbp comes from gettdb() called with sproto passed as parameter.
This has been discussed before.  In bridge_ipsec() tdb comes from
gettdb() called with proto.  There we goto skiplookup if proto !=
IPPROTO_ESP && proto != IPPROTO_AH && proto != IPPROTO_IPCOMP.

The question is, do we trust this analysis?

bluhm

Re: [patch] mg: fix overflow on vteeol()

[Hiltjo Posthuma]

On Sun, May 07, 2017 at 05:31:26PM +0200, Hiltjo Posthuma wrote:
> Hey,
> 
> mg crashes with certain (unicode) characters and moving the cursor to the
> end of the line.
> 
> The characters are printed to the screen as \nnn in vtpute() and vtcol is
> updated, however vteeol() will write beyond the buffer.
> 
> A test file contains the data:
> 
> ——
> 
> It is printed to the screen as: \342\200\224\342... etc.
> 
> It is safer to use vtpute() here because it checks if vtcol >= 0.
> 
> Below is a patch:
> 
> 
> diff --git a/usr.bin/mg/display.c b/usr.bin/mg/display.c
> index 7af723ce268..d7c22554753 100644
> --- a/usr.bin/mg/display.c
> +++ b/usr.bin/mg/display.c
> @@ -381,11 +381,8 @@ vtpute(int c)
>  void
>  vteeol(void)
>  {
> - struct video *vp;
> -
> - vp = vscreen[vtrow];
>   while (vtcol < ncol)
> - vp->v_text[vtcol++] = ' ';
> + vtpute(' ');
>  }
>  
>  /*
> 

Any OKs?

-- 
Kind regards,
Hiltjo

Re: Atomic copyin(9)/copyout(9) for amd64

[Martin Pieuchot]

On 01/05/17(Mon) 18:02, Mark Kettenis wrote:
> The futex(2) syscall needs to be able to atomically copy the futex in
> and out of userland.  The current implementation uses copyin(9) and
> copyout(9) for that.  The futex is a 32-bit integer, and currently our
> copyin(9) and copyout(9) don't guarantee an atomic 32-bit access.
> Previously mpi@ and I discussed implementing new interfaces that do
> guarantee the required atomicity.  However, it oocurred to me that we
> could simply change our copyin implementations such that they
> guarantee atomicity of a properly aligned 32-bit copyin and copyout.
> 
> The i386 version of these calls uses "rep movsl", which means it is
> already atomic.  At least that is how I interpret 8.2.4 in Volume 3A
> of the Intel SDM.  The diff below makes the amd64 version safe as
> well.  This does introduce a few additional instructions in the loop.
> Apparently modern Intel CPUs optimize the string loops.  If we can
> rely on the hardware to turn 32-bit moves into 64-bit moves, we could
> simplify the code by using "rep movsl" instead of "rep movsq".
> 
> Thoughts?

I'm ok with this approach.  I think you should document it though.

Now if others think it's better to introduce a new function I'm fine
with that too but this can be done afterward. 

> Index: arch/amd64/amd64/copy.S
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/copy.S,v
> retrieving revision 1.7
> diff -u -p -r1.7 copy.S
> --- arch/amd64/amd64/copy.S   25 Apr 2015 21:31:24 -  1.7
> +++ arch/amd64/amd64/copy.S   1 May 2017 15:32:17 -
> @@ -138,7 +138,12 @@ ENTRY(copyout)
>   rep
>   movsq
>   movb%al,%cl
> - andb$7,%cl
> + shrb$2,%cl
> + andb$1,%cl
> + rep
> + movsl
> + movb%al,%cl
> + andb$3,%cl
>   rep
>   movsb
>   SMAP_CLAC
> @@ -168,7 +173,12 @@ ENTRY(copyin)
>   rep
>   movsq
>   movb%al,%cl
> - andb$7,%cl
> + shrb$2,%cl
> + andb$1,%cl
> + rep
> + movsl
> + movb%al,%cl
> + andb$3,%cl
>   rep
>   movsb
>  
> 

Replace memset with explicit_bzero on user(8)

[Ricardo Mestre]

Hi,

I'm almost sure I sent this aeons ago already, but never got commited.

It calls explicit_bzero instead of memset on passwords as soon as they're not
needed in memory anymore.

Index: user.c
===
RCS file: /cvs/src/usr.sbin/user/user.c,v
retrieving revision 1.118
diff -u -p -u -r1.118 user.c
--- user.c  30 Nov 2016 23:58:07 -  1.118
+++ user.c  12 May 2017 07:56:11 -
@@ -1359,7 +1359,7 @@ moduser(char *login_name, char *newlogin
up->u_flags |= F_PASSWORD;
memsave(>u_password, pwp->pw_passwd,
strlen(pwp->pw_passwd));
-   memset(pwp->pw_passwd, 'X', strlen(pwp->pw_passwd));
+   explicit_bzero(pwp->pw_passwd, strlen(pwp->pw_passwd));
}
}
endpwent();
@@ -1788,7 +1788,7 @@ useradd(int argc, char **argv)
break;
case 'p':
memsave(_password, optarg, strlen(optarg));
-   memset(optarg, 'X', strlen(optarg));
+   explicit_bzero(optarg, strlen(optarg));
break;
case 'r':
defaultfield = 1;
@@ -1929,7 +1929,7 @@ usermod(int argc, char **argv)
break;
case 'p':
memsave(_password, optarg, strlen(optarg));
-   memset(optarg, 'X', strlen(optarg));
+   explicit_bzero(optarg, strlen(optarg));
u.u_flags |= F_PASSWORD;
break;
case 's':