Re: Reenable ASN1_DN IDs with certificates in iked

[Tim Stewart]

Here is a version of the previous patch that preserves tabs properly.
Apologies.

-TimS


Index: parse.y
===
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.65
diff -u -p -r1.65 parse.y
--- parse.y 24 Apr 2017 07:07:25 -  1.65
+++ parse.y 17 May 2017 05:40:39 -
@@ -1735,6 +1735,8 @@ set_policy_auth_method(const char *peeri
method = IKEV2_AUTH_NONE;
cert_type = IKEV2_CERT_NONE;

+   ikeauth = >pol_auth;
+
if (key != NULL) {
/* infer policy from key type */
if ((rsa = EVP_PKEY_get1_RSA(key)) != NULL) {
@@ -1767,14 +1769,16 @@ set_policy_auth_method(const char *peeri

if (method == IKEV2_AUTH_NONE || cert_type == IKEV2_CERT_NONE)
return (-1);
+   } else if (ikeauth->auth_method == IKEV2_AUTH_RSA_SIG) {
+   /* default to IKEV2_CERT_X509_CERT otherwise */
+   method = IKEV2_AUTH_RSA_SIG;
+   cert_type = IKEV2_CERT_X509_CERT;
} else {
/* default to IKEV2_CERT_X509_CERT otherwise */
method = IKEV2_AUTH_SIG;
cert_type = IKEV2_CERT_X509_CERT;
}

-   ikeauth = >pol_auth;
-
if (ikeauth->auth_method == IKEV2_AUTH_SHARED_KEY_MIC) {
if (key != NULL &&
method != IKEV2_AUTH_RSA_SIG)
@@ -1784,6 +1788,7 @@ set_policy_auth_method(const char *peeri

if (ikeauth->auth_method != IKEV2_AUTH_NONE &&
ikeauth->auth_method != IKEV2_AUTH_SIG_ANY &&
+   ikeauth->auth_method != IKEV2_AUTH_RSA_SIG &&
ikeauth->auth_method != method)
goto mismatch;

Add Diffie-Hellman group negotiation to iked

[Tim Stewart]

This patch teaches iked to reject a KE with a Notify payload of type
INVALID_KE_PAYLOAD when the KE uses a different Diffie-Hellman group
than is configured locally.  The rejection indicates the desired group.

In my environment, this patch allows stock strongSwan on Android from
the Google Play store to interop with iked.  strongSwan's logs show the
following once iked is patched:

  [IKE] initiating IKE_SA android[7] to 192.0.2.1
  [ENC] generating IKE_SA_INIT request 0 [ SA KE No N(NATD_S_IP) N(NATD_D_IP) 
N(FRAG_SUP) N(HASH_ALG) N(REDIR_SUP) ]
  [ENC] parsed IKE_SA_INIT response 0 [ N(INVAL_KE) ]
  [IKE] peer didn't accept DH group ECP_256, it requested MODP_2048
  [IKE] initiating IKE_SA android[7] to 192.0.2.1
  [ENC] generating IKE_SA_INIT request 0 [ SA KE No N(NATD_S_IP) N(NATD_D_IP) 
N(FRAG_SUP) N(HASH_ALG) N(REDIR_SUP) ]
  [ENC] parsed IKE_SA_INIT response 0 [ SA KE No N(NATD_S_IP) N(NATD_D_IP) 
CERTREQ N(HASH_ALG) ]

I'm happy to iterate on this patch to get it into proper shape for
inclusion.

-TimS


Index: iked.h
===
RCS file: /cvs/src/sbin/iked/iked.h,v
retrieving revision 1.115
diff -u -p -r1.115 iked.h
--- iked.h  26 Apr 2017 10:42:38 -  1.115
+++ iked.h  17 May 2017 05:21:24 -
@@ -502,6 +502,7 @@ struct iked_message {
struct iked_proposalsmsg_proposals;
struct iked_spi  msg_rekey;
struct ibuf *msg_nonce; /* dh NONCE */
+   uint16_t msg_dhgroup;   /* dh group */
struct ibuf *msg_ke;/* dh key exchange */
struct iked_id   msg_auth;  /* AUTH payload */
struct iked_id   msg_id;
Index: ikev2.c
===
RCS file: /cvs/src/sbin/iked/ikev2.c,v
retrieving revision 1.154
diff -u -p -r1.154 ikev2.c
--- ikev2.c 26 Apr 2017 10:42:38 -  1.154
+++ ikev2.c 17 May 2017 05:21:24 -
@@ -71,6 +71,8 @@ intikev2_init_done(struct iked *, stru
 voidikev2_resp_recv(struct iked *, struct iked_message *,
struct ike_header *);
 int ikev2_resp_ike_sa_init(struct iked *, struct iked_message *);
+int ikev2_resp_ike_invalid_ke(struct iked *, struct iked_message *,
+   struct iked_kex *);
 int ikev2_resp_ike_auth(struct iked *, struct iked_sa *);
 int ikev2_resp_ike_eap(struct iked *, struct iked_sa *, struct ibuf *);
 int ikev2_send_auth_failed(struct iked *, struct iked_sa *);
@@ -96,8 +98,8 @@ intikev2_sa_responder(struct iked *, s
struct iked_message *);
 int ikev2_sa_initiator_dh(struct iked_sa *, struct iked_message *,
unsigned int);
-int ikev2_sa_responder_dh(struct iked_kex *, struct iked_proposals *,
-   struct iked_message *, unsigned int);
+int ikev2_sa_responder_dh(struct iked *, struct iked_kex *,
+   struct iked_proposals *, struct iked_message *, unsigned int);
 voidikev2_sa_cleanup_dh(struct iked_sa *);
 int ikev2_sa_keys(struct iked *, struct iked_sa *, struct ibuf *);
 int ikev2_sa_tag(struct iked_sa *, struct iked_id *);
@@ -2279,6 +2281,84 @@ ikev2_resp_ike_sa_init(struct iked *env,
 }

 int
+ikev2_resp_ike_invalid_ke(struct iked *env, struct iked_message *msg,
+struct iked_kex *kex)
+{
+   struct iked_message  resp;
+   struct ike_header   *hdr;
+   struct ikev2_payload*pld;
+   struct ikev2_notify *n;
+   struct iked_sa  *sa = msg->msg_sa;
+   struct ibuf *buf;
+   uint8_t *ptr;
+   ssize_t  len;
+   uint16_t group;
+   int  ret = -1;
+
+   if (sa->sa_hdr.sh_initiator) {
+   log_debug("%s: called by initiator", __func__);
+   return (-1);
+   }
+
+   log_debug("%s: rejecting with INVALID_KE_PAYLOAD", __func__);
+
+   if ((buf = ikev2_msg_init(env, ,
+   >msg_peer, msg->msg_peerlen,
+   >msg_local, msg->msg_locallen, 1)) == NULL)
+   goto done;
+
+   resp.msg_sa = sa;
+   resp.msg_fd = msg->msg_fd;
+   resp.msg_natt = msg->msg_natt;
+   resp.msg_msgid = 0;
+
+   /* IKE header */
+   if ((hdr = ikev2_add_header(buf, sa, resp.msg_msgid,
+   IKEV2_PAYLOAD_NOTIFY, IKEV2_EXCHANGE_IKE_SA_INIT,
+   IKEV2_FLAG_RESPONSE)) == NULL)
+   goto done;
+
+   /* NOTIFY payload */
+   if ((pld = ikev2_add_payload(buf)) == NULL)
+   goto done;
+   if ((n = ibuf_advance(buf, sizeof(*n))) == NULL)
+   goto done;
+   n->n_protoid = 0;
+   n->n_spisize = 0;
+   n->n_type = htobe16(IKEV2_N_INVALID_KE_PAYLOAD);
+
+   /* add desired dh group to NOTIFY */
+   group = htobe16(kex->kex_dhgroup->id);
+   len = 

Reenable ASN1_DN IDs with certificates in iked

[Tim Stewart]

A sample configuration:

ikev2 "win10host" passive esp \
  from 0.0.0.0/0 to 10.1.1.51 \
  local any peer any \
  ikesa auth hmac-sha2-384 enc aes-256 prf hmac-sha2-384 group modp2048 \
  childsa enc aes-256-gcm group modp2048 \
  srcid "/C=US/ST=New York/L=NYC/O=Stoo Labs/OU=iked/CN=foo.stoo.org" \
  dstid "/C=US/ST=New York/L=NYC/O=Stoo Labs/OU=iked/CN=bar.stoo.org" \
  rsa \
  config address 10.1.1.51 \
  config name-server 10.1.1.5 \
  config name-server 10.1.1.6 \
  tag "$name-$id"

The above configuration worked fine with iked in OpenBSD 6.0.  It broke
as of 6.1 with the following error:

set_policy_auth_method: ikeauth policy mismatch, rsa specified, but only 
rfc7427 possible
set_policy: failed to set policy auth method for
/etc/iked.conf: 17: create_ike failed
/etc/iked.conf: no valid configuration rules found

I use a CA certificate and signed host certificates generated using a
process like the EXAMPLES section in ikectl(8).  I'm a bit surprised
that I could not find anyone else who has seen this problem, so maybe
I'm doing something strange without realizing it.

The following patch restores the old functionality, though I include it
mainly for demonstration purposes.  I'm happy to improve it and
resubmit, depending on feedback.

-TimS


Index: parse.y
===
RCS file: /cvs/src/sbin/iked/parse.y,v
retrieving revision 1.65
diff -u -p -r1.65 parse.y
--- parse.y 24 Apr 2017 07:07:25 -  1.65
+++ parse.y 17 May 2017 04:58:34 -
@@ -1735,6 +1735,8 @@ set_policy_auth_method(const char *peeri
method = IKEV2_AUTH_NONE;
cert_type = IKEV2_CERT_NONE;

+   ikeauth = >pol_auth;
+
if (key != NULL) {
/* infer policy from key type */
if ((rsa = EVP_PKEY_get1_RSA(key)) != NULL) {
@@ -1767,14 +1769,16 @@ set_policy_auth_method(const char *peeri

if (method == IKEV2_AUTH_NONE || cert_type == IKEV2_CERT_NONE)
return (-1);
+   } else if (ikeauth->auth_method == IKEV2_AUTH_RSA_SIG) {
+   /* default to IKEV2_CERT_X509_CERT otherwise */
+   method = IKEV2_AUTH_RSA_SIG;
+   cert_type = IKEV2_CERT_X509_CERT;
} else {
/* default to IKEV2_CERT_X509_CERT otherwise */
method = IKEV2_AUTH_SIG;
cert_type = IKEV2_CERT_X509_CERT;
}

-   ikeauth = >pol_auth;
-
if (ikeauth->auth_method == IKEV2_AUTH_SHARED_KEY_MIC) {
if (key != NULL &&
method != IKEV2_AUTH_RSA_SIG)
@@ -1784,6 +1788,7 @@ set_policy_auth_method(const char *peeri

if (ikeauth->auth_method != IKEV2_AUTH_NONE &&
ikeauth->auth_method != IKEV2_AUTH_SIG_ANY &&
+   ikeauth->auth_method != IKEV2_AUTH_RSA_SIG &&
ikeauth->auth_method != method)
goto mismatch;

better inline asm for spllower on hppa

[David Gwynne]

this simplifies the asm in spllower.

the trap that break forces to run reads the new cpl value out of
arg0 (r26) and puts the old valud in r28 (ret0). the current asm
is convoluted in how it gets the compiler to avoid those regs across
the asm.

right now it has gcc place the ncpl value in ret0, and uses asm to
copy that into arg0 before the break. it lists arg0 as a clobber
so gcc will avoid it before running this code. it also marks ret0
as writable, which is correct.

this moves to using two register asm variables, one each for arg0
and ret0, and only uses them as input and output operands respectively.
this lets gcc place the ncpl value in the input register, which it
does a good job of, and it avoids the extra copy operation.

ive only used this as part of a bigger diff. im compiling a kernel
with just it now, but it takes ages. 180MHz cpus arent as good as
i remember them being.

ok?

Index: include/intr.h
===
RCS file: /cvs/src/sys/arch/hppa/include/intr.h,v
retrieving revision 1.42
diff -u -p -r1.42 intr.h
--- include/intr.h  13 Sep 2015 14:58:20 -  1.42
+++ include/intr.h  17 May 2017 03:58:32 -
@@ -92,11 +92,15 @@ voidintr_barrier(void *);
 static __inline int
 spllower(int ncpl)
 {
-   register int ocpl asm("r28") = ncpl;
-   __asm volatile("copy  %0, %%arg0\n\tbreak %1, %2"
-   : "+r" (ocpl) : "i" (HPPA_BREAK_KERNEL), "i" (HPPA_BREAK_SPLLOWER)
-   : "r26", "memory");
-   return (ocpl);
+   register int arg0 asm("r26") = ncpl;
+   register int ret0 asm("r28");
+
+   __asm volatile("break %1, %2"
+   : "=r" (ret0)
+   : "i" (HPPA_BREAK_KERNEL), "i" (HPPA_BREAK_SPLLOWER), "r" (arg0)
+   : "memory");
+
+   return (ret0);
 }
 
 static __inline int

Re: pf queue definition: bandwidth resolution problem

[Mike Belopuhov]

On Tue, May 16, 2017 at 18:19 +, Carl Mascott wrote:
> At the end is the patch I mentioned against pftop.c v1.37, using a guard 
> digit.
> WARNING: Untested. I couldn't even try to compile it.
> 
> At this point I don't see anything wrong with your patch, Mike.
> 
> 
> 
> On Mon, 5/15/17, Mike Belopuhov  wrote:
> 
>  Subject: Re: pf queue definition: bandwidth resolution problem
>  To: "Carl Mascott" 
>  Cc: "Theo Buehler" , tech@openbsd.org
>  Date: Monday, May 15, 2017, 3:28 PM
>  
>  On Sun, May 14, 2017 at 21:08 +, Carl
>  Mascott wrote:
>  > OK, I was indeed missing
>  something. Thanks, Theo and Mike.
>  > 
>  > I think I see another very minor problem,
>  though.
>  > Let the pf BW be 9,999,999.
>  > Shift right 3 digits (divide by 1000) :
>  yields 9,999K.
>  > (If we were doing
>  floating point arithmetic, would yield 9,999.999K.)
>  > You (Mike) don't round this,
>  presumably to avoid overflow to 10,000 (5 digits).
>  > However, since (i < 3) it could have
>  been rounded and scaled again, to 10M.
>  >
>  Slightly more accurate but not a big deal.
>  >
>  
>  You're right, initially I had a "<=
>  " there but then I got
>  sidetracked
>  with a hypothetical problem that right now seems
>  like my imagination.  I've double checked
>  numbers and indeed,
>  it should be "if
>  (rtmp <= )".
>  
>  > 
>  >
>  
>  > On Sun, 5/14/17, Theo Buehler 
>  wrote:
>  > 
>  >  Subject:
>  Re: pf queue definition: bandwidth resolution problem
>  >  To: tech@openbsd.org
>  >  Date: Sunday, May 14, 2017, 4:35 PM
>  >  
>  >  On Sun, May 14,
>  2017 at 08:29:18PM +, Carl
>  > 
>  Mascott wrote:
>  >  > It looks to me
>  like you
>  >  are rounding on each
>  iteration of the for-loop:
>  >  > 
>  >  > +    for (i = 0;
>  >  rate >  && i <= 3;
>  i++) {
>  >  > +        rtmp = rate
>  / 1000;
>  >  > +        if (rtmp
>  < )
>  >  
>  > 
>  This is only true in the last
>  > 
>  iteration.
>  >  
>  > 
>  > +            rtmp
>  >  += (rate
>  % 1000) / 500;
>  >  > +       
>  >  rate = rtmp;
>  >  >
>  +    }
>  >  > 
>  > 
>  > Am I missing
>  >  something?
>  >  
>  >  
>  > 
>  
>  --- pftop.c.old  2017-05-14 09:51:58.620737200 -0400
> +++ pftop.c   2017-05-16 14:01:08.184811800 -0400
> @@ -1608,7 +1608,7 @@
>  void
>  print_queue_node(struct pfctl_queue_node *node)
>  {
> - u_int   rate;
> + u_int64_t   rate;
>   int i;
>   double  interval, pps, bps;
>   static const char unit[] = " KMG";
> @@ -1624,9 +1624,22 @@
>   // XXX: missing min, max, burst
>   tb_start();
>   rate = node->qs.linkshare.m2.absolute;
> - for (i = 0; rate >= 1000 && i <= 3; i++)
> - rate /= 1000;
> - tbprintf("%u%c", rate, unit[i]);
> + rate *= 10UL;
> + for (i = 0; rate > 9UL && i <= 3; i++)
> + rate /= 1000UL;
> + if (rate % 10UL >= 5UL) {
> + rate += 10UL;   /* LSD incorrect, don't care */
> + if (rate > 9UL) {
> + if (i < 3) {
> + rate /= 1000UL;
> + i++;
> + }
> + else
> + rate -= 10UL;
> + }
> + }
> + rate /= 10UL;
> + tbprintf("%u%c", (unsigned) rate, unit[i]);
>   print_fld_tb(FLD_BANDW);
>  
>   if (node->qstats.valid && node->qstats_last.valid)

Thanks for the diff!  But I must say this is a bit of an overkill (-:
I've checked in my amended diff just now.  Thanks for reporting and
helping with the fix!

Re: copyin32(9) for i386 and amd64

[Ted Unangst]

Mark Kettenis wrote:
> We can just call copyin(9) since it already is atomic.  But check
> whether the userland futex is properly aligned and return EFAULT if it
> isn't such that this system call behaves like it does on strict
> alignment architectures.

hmm. do we want this? i understand the appeal, but due to differing
compilers/etc, some structs that are carefully packed on some platforms may
not be aligned on i386. however, they would be correctly aligned where
required.

are we trying to prevent a problem that doesn't exist?
but not a major objection.

inet protosw #ifdef cleanup

[Alexander Bluhm]

Hi,

The large and nested GIF #ifdefs in protosw make it hard to figure
out what is going on.  There are also some inconsistencies 
that seem to be oversights.

So I would like to make the #ifdef more specific.

This has been changed:
- In the GIF case, IPPROTO_IPV6 in inetsw got a rip_ctloutput.
- In the non GIF case, IPPROTO_IPV6 in inet6sw lost ipip_sysctl.

ok?

bluhm

Index: netinet/in_proto.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_proto.c,v
retrieving revision 1.77
diff -u -p -r1.77 in_proto.c
--- netinet/in_proto.c  9 May 2017 13:33:50 -   1.77
+++ netinet/in_proto.c  16 May 2017 17:13:50 -
@@ -231,78 +231,62 @@ struct protosw inetsw[] = {
   .pr_init = icmp_init,
   .pr_sysctl   = icmp_sysctl
 },
-#if NGIF > 0
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
   .pr_protocol = IPPROTO_IPV4,
   .pr_flags= PR_ATOMIC|PR_ADDR,
+#if NGIF > 0
   .pr_input= in_gif_input,
+#else
+  .pr_input= ip4_input,
+#endif
   .pr_ctloutput= rip_ctloutput,
   .pr_usrreq   = rip_usrreq,
   .pr_attach   = rip_attach,
   .pr_sysctl   = ipip_sysctl,
   .pr_init = ipip_init
 },
-{
-  .pr_type = SOCK_RAW,
-  .pr_domain   = ,
-  .pr_protocol = IPPROTO_ETHERIP,
-  .pr_flags= PR_ATOMIC|PR_ADDR,
-  .pr_input= etherip_input,
-  .pr_ctloutput= rip_ctloutput,
-  .pr_usrreq   = rip_usrreq,
-  .pr_attach   = rip_attach,
-  .pr_sysctl   = etherip_sysctl
-},
 #ifdef INET6
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
   .pr_protocol = IPPROTO_IPV6,
   .pr_flags= PR_ATOMIC|PR_ADDR,
+#if NGIF > 0
   .pr_input= in_gif_input,
+#else
+  .pr_input= ip4_input,
+#endif
+  .pr_ctloutput= rip_ctloutput,
   .pr_usrreq   = rip_usrreq, /* XXX */
   .pr_attach   = rip_attach
 },
 #endif
-#ifdef MPLS
+#if NGIF > 0
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
-  .pr_protocol = IPPROTO_MPLS,
+  .pr_protocol = IPPROTO_ETHERIP,
   .pr_flags= PR_ATOMIC|PR_ADDR,
   .pr_input= etherip_input,
-  .pr_usrreq   = rip_usrreq,
-  .pr_attach   = rip_attach
-},
-#endif
-#else /* NGIF */
-{
-  .pr_type = SOCK_RAW,
-  .pr_domain   = ,
-  .pr_protocol = IPPROTO_IPIP,
-  .pr_flags= PR_ATOMIC|PR_ADDR,
-  .pr_input= ip4_input,
   .pr_ctloutput= rip_ctloutput,
   .pr_usrreq   = rip_usrreq,
   .pr_attach   = rip_attach,
-  .pr_sysctl   = ipip_sysctl,
-  .pr_init = ipip_init
+  .pr_sysctl   = etherip_sysctl
 },
-#ifdef INET6
+#endif /* NGIF */
+#if defined(MPLS) && NGIF > 0
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
-  .pr_protocol = IPPROTO_IPV6,
+  .pr_protocol = IPPROTO_MPLS,
   .pr_flags= PR_ATOMIC|PR_ADDR,
-  .pr_input= ip4_input,
-  .pr_ctloutput= rip_ctloutput,
-  .pr_usrreq   = rip_usrreq, /* XXX */
+  .pr_input= etherip_input,
+  .pr_usrreq   = rip_usrreq,
   .pr_attach   = rip_attach
 },
-#endif
-#endif /*NGIF*/
+#endif /* MPLS && GIF */
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
Index: netinet6/in6_proto.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_proto.c,v
retrieving revision 1.92
diff -u -p -r1.92 in6_proto.c
--- netinet6/in6_proto.c9 May 2017 13:33:50 -   1.92
+++ netinet6/in6_proto.c16 May 2017 21:59:21 -
@@ -236,61 +236,47 @@ struct protosw inet6sw[] = {
   .pr_sysctl   = ipcomp_sysctl
 },
 #endif /* IPSEC */
-#if NGIF > 0
-{
-  .pr_type = SOCK_RAW,
-  .pr_domain   = ,
-  .pr_protocol = IPPROTO_ETHERIP,
-  .pr_flags= PR_ATOMIC|PR_ADDR,
-  .pr_input= etherip_input,
-  .pr_ctloutput= rip6_ctloutput,
-  .pr_usrreq   = rip6_usrreq,
-  .pr_attach   = rip6_attach,
-  .pr_sysctl   = etherip_sysctl
-},
-{
-  .pr_type = SOCK_RAW,
-  .pr_domain   = ,
-  .pr_protocol = IPPROTO_IPV6,
-  .pr_flags= PR_ATOMIC|PR_ADDR,
-  .pr_input= in6_gif_input,
-  .pr_ctloutput= rip6_ctloutput,
-  .pr_usrreq   = rip6_usrreq,  /* XXX */
-  .pr_attach   = rip6_attach
-},
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
   .pr_protocol = IPPROTO_IPV4,
   .pr_flags= PR_ATOMIC|PR_ADDR,
+#if NGIF > 0
   .pr_input= in6_gif_input,
+#else
+  .pr_input= ip4_input,
+#endif
   .pr_ctloutput= rip6_ctloutput,
   .pr_usrreq   = rip6_usrreq,  /* XXX */
   .pr_attach   = rip6_attach
 },
-#else /* NGIF */
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
   .pr_protocol = IPPROTO_IPV6,
   .pr_flags= PR_ATOMIC|PR_ADDR,
+#if NGIF > 0
+  .pr_input= in6_gif_input,
+#else
   .pr_input= ip4_input,
+#endif
   .pr_ctloutput= rip6_ctloutput,
   .pr_usrreq   = rip6_usrreq,  /* XXX */
   .pr_attach   = rip6_attach,
-  .pr_sysctl   = ipip_sysctl
 },
+#if NGIF > 0
 {
   .pr_type = SOCK_RAW,
   .pr_domain   = ,
-  .pr_protocol = IPPROTO_IPV4,
+  .pr_protocol = IPPROTO_ETHERIP,
   .pr_flags= PR_ATOMIC|PR_ADDR,
-  .pr_input= ip4_input,
+  .pr_input= etherip_input,
   

copyin32(9) for i386 and amd64

[Mark Kettenis]

We can just call copyin(9) since it already is atomic.  But check
whether the userland futex is properly aligned and return EFAULT if it
isn't such that this system call behaves like it does on strict
alignment architectures.

Also, add a prototype to  such that we can actually use it.

ok?


Index: sys/systm.h
===
RCS file: /cvs/src/sys/sys/systm.h,v
retrieving revision 1.129
diff -u -p -r1.129 systm.h
--- sys/systm.h 15 May 2017 12:26:00 -  1.129
+++ sys/systm.h 16 May 2017 21:41:59 -
@@ -207,6 +207,7 @@ int copyoutstr(const void *, void *, siz
 intcopyin(const void *, void *, size_t)
__attribute__ ((__bounded__(__buffer__,2,3)));
 intcopyout(const void *, void *, size_t);
+intcopyin32(const uint32_t *, uint32_t *);
 
 void   arc4random_buf(void *, size_t)
__attribute__ ((__bounded__(__buffer__,1,2)));
Index: arch/i386/i386/machdep.c
===
RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.600
diff -u -p -r1.600 machdep.c
--- arch/i386/i386/machdep.c30 Apr 2017 16:45:45 -  1.600
+++ arch/i386/i386/machdep.c16 May 2017 21:41:59 -
@@ -3890,6 +3890,16 @@ splassert_check(int wantipl, const char 
 }
 #endif
 
+int
+copyin32(const uint32_t *uaddr, uint32_t *kaddr)
+{
+   if ((vaddr_t)uaddr & 0x3)
+   return EFAULT;
+
+   /* copyin(9) is atomic */
+   return copyin(uaddr, kaddr, sizeof(uint32_t));
+}
+
 /*
  * True if the system has any non-level interrupts which are shared
  * on the same pin.
Index: arch/amd64/amd64/machdep.c
===
RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.228
diff -u -p -r1.228 machdep.c
--- arch/amd64/amd64/machdep.c  30 Apr 2017 16:45:45 -  1.228
+++ arch/amd64/amd64/machdep.c  16 May 2017 21:41:59 -
@@ -1777,6 +1777,16 @@ splassert_check(int wantipl, const char 
 }
 #endif
 
+int
+copyin32(const uint32_t *uaddr, uint32_t *kaddr)
+{
+   if ((vaddr_t)uaddr & 0x3)
+   return EFAULT;
+
+   /* copyin(9) is atomic */
+   return copyin(uaddr, kaddr, sizeof(uint32_t));
+}
+
 void
 getbootinfo(char *bootinfo, int bootinfo_size)
 {

Minor annoyances in the pfctl queue parser

[Mike Belopuhov]

jmc@ has pointed out some inconsistencies in how bandwidth parameters
are parsed and printed out. I agree that we can fix or improve this:

1) Bit/B/bit/b are parsed incorrectly. Ditching them is another option.

2) Should we make lowercase K, M and G parseable as well? I'd like that.

3) Avoid an extra space when printing out bandwidth and there's no unit
   (K/M/G) to print.

OK?

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index 63aaafeeea5..8b0f1d3182b 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -1455,24 +1455,21 @@ bandwidth   : STRING {
 
bps = strtod($1, );
if (cp != NULL) {
if (strlen(cp) > 1) {
char *cu = cp + 1;
-   if (!strcmp(cu, "Bit") ||
-   !strcmp(cu, "B") ||
-   !strcmp(cu, "bit") ||
-   !strcmp(cu, "b")) {
+   if (!strcmp(cp, "Bit") ||
+   !strcmp(cp, "bit"))
*cu = 0;
-   }
}
-   if (!strcmp(cp, "b"))
+   if (!strcasecmp(cp, "B"))
; /* nothing */
-   else if (!strcmp(cp, "K"))
+   else if (!strcasecmp(cp, "K"))
bps *= 1000;
-   else if (!strcmp(cp, "M"))
+   else if (!strcasecmp(cp, "M"))
bps *= 1000 * 1000;
-   else if (!strcmp(cp, "G"))
+   else if (!strcasecmp(cp, "G"))
bps *= 1000 * 1000 * 1000;
else if (!strcmp(cp, "%")) {
if (bps < 0 || bps > 100) {
yyerror("bandwidth spec "
"out of range");
diff --git sbin/pfctl/pfctl_parser.c sbin/pfctl/pfctl_parser.c
index a69acb2e5b2..e468c2cfe1d 100644
--- sbin/pfctl/pfctl_parser.c
+++ sbin/pfctl/pfctl_parser.c
@@ -1168,19 +1168,19 @@ print_tabledef(const char *name, int flags, int addrs,
 void
 print_bwspec(const char *prefix, struct pf_queue_bwspec *bw)
 {
u_int   rate;
int i;
-   static const char unit[] = " KMG";
+   static const char *unit[] = { "", "K", "M", "G" };
 
if (bw->percent)
printf("%s%u%%", prefix, bw->percent);
else if (bw->absolute) {
rate = bw->absolute;
for (i = 0; rate >= 1000 && i <= 3 && (rate % 1000 == 0); i++)
rate /= 1000;
-   printf("%s%u%c", prefix, rate, unit[i]);
+   printf("%s%u%s", prefix, rate, unit[i]);
}
 }
 
 void
 print_scspec(const char *prefix, struct pf_queue_scspec *sc)

Re: pf queue definition: bandwidth resolution problem

[Carl Mascott]

At the end is the patch I mentioned against pftop.c v1.37, using a guard digit.
WARNING: Untested. I couldn't even try to compile it.

At this point I don't see anything wrong with your patch, Mike.



On Mon, 5/15/17, Mike Belopuhov  wrote:

 Subject: Re: pf queue definition: bandwidth resolution problem
 To: "Carl Mascott" 
 Cc: "Theo Buehler" , tech@openbsd.org
 Date: Monday, May 15, 2017, 3:28 PM
 
 On Sun, May 14, 2017 at 21:08 +, Carl
 Mascott wrote:
 > OK, I was indeed missing
 something. Thanks, Theo and Mike.
 > 
 > I think I see another very minor problem,
 though.
 > Let the pf BW be 9,999,999.
 > Shift right 3 digits (divide by 1000) :
 yields 9,999K.
 > (If we were doing
 floating point arithmetic, would yield 9,999.999K.)
 > You (Mike) don't round this,
 presumably to avoid overflow to 10,000 (5 digits).
 > However, since (i < 3) it could have
 been rounded and scaled again, to 10M.
 >
 Slightly more accurate but not a big deal.
 >
 
 You're right, initially I had a "<=
 " there but then I got
 sidetracked
 with a hypothetical problem that right now seems
 like my imagination.  I've double checked
 numbers and indeed,
 it should be "if
 (rtmp <= )".
 
 > 
 >
 
 > On Sun, 5/14/17, Theo Buehler 
 wrote:
 > 
 >  Subject:
 Re: pf queue definition: bandwidth resolution problem
 >  To: tech@openbsd.org
 >  Date: Sunday, May 14, 2017, 4:35 PM
 >  
 >  On Sun, May 14,
 2017 at 08:29:18PM +, Carl
 > 
 Mascott wrote:
 >  > It looks to me
 like you
 >  are rounding on each
 iteration of the for-loop:
 >  > 
 >  > +    for (i = 0;
 >  rate >  && i <= 3;
 i++) {
 >  > +        rtmp = rate
 / 1000;
 >  > +        if (rtmp
 < )
 >  
 > 
 This is only true in the last
 > 
 iteration.
 >  
 > 
 > +            rtmp
 >  += (rate
 % 1000) / 500;
 >  > +       
 >  rate = rtmp;
 >  >
 +    }
 >  > 
 > 
 > Am I missing
 >  something?
 >  
 >  
 > 
 
 --- pftop.c.old2017-05-14 09:51:58.620737200 -0400
+++ pftop.c 2017-05-16 14:01:08.184811800 -0400
@@ -1608,7 +1608,7 @@
 void
 print_queue_node(struct pfctl_queue_node *node)
 {
-   u_int   rate;
+   u_int64_t   rate;
int i;
double  interval, pps, bps;
static const char unit[] = " KMG";
@@ -1624,9 +1624,22 @@
// XXX: missing min, max, burst
tb_start();
rate = node->qs.linkshare.m2.absolute;
-   for (i = 0; rate >= 1000 && i <= 3; i++)
-   rate /= 1000;
-   tbprintf("%u%c", rate, unit[i]);
+   rate *= 10UL;
+   for (i = 0; rate > 9UL && i <= 3; i++)
+   rate /= 1000UL;
+   if (rate % 10UL >= 5UL) {
+   rate += 10UL;   /* LSD incorrect, don't care */
+   if (rate > 9UL) {
+   if (i < 3) {
+   rate /= 1000UL;
+   i++;
+   }
+   else
+   rate -= 10UL;
+   }
+   }
+   rate /= 10UL;
+   tbprintf("%u%c", (unsigned) rate, unit[i]);
print_fld_tb(FLD_BANDW);
 
if (node->qstats.valid && node->qstats_last.valid)

Re: gif(4) vs splnet()

[Alexander Bluhm]

On Mon, May 15, 2017 at 03:11:20PM +0200, Martin Pieuchot wrote:
> Similar to gre(4), the global list of interfaces needs to be protected
> by the NET_LOCK().
> 
> ok?

OK bluhm@

> 
> Index: net/if_gif.c
> ===
> RCS file: /cvs/src/sys/net/if_gif.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 if_gif.c
> --- net/if_gif.c  4 May 2017 15:00:24 -   1.94
> +++ net/if_gif.c  15 May 2017 13:10:13 -
> @@ -129,9 +129,9 @@ gif_clone_create(struct if_clone *ifc, i
>  #if NBPFILTER > 0
>   bpfattach(>gif_if.if_bpf, >gif_if, DLT_LOOP, sizeof(u_int32_t));
>  #endif
> - s = splnet();
> + NET_LOCK(s);
>   LIST_INSERT_HEAD(_softc_list, sc, gif_list);
> - splx(s);
> + NET_UNLOCK(s);
>  
>   return (0);
>  }
> @@ -142,9 +142,9 @@ gif_clone_destroy(struct ifnet *ifp)
>   struct gif_softc *sc = ifp->if_softc;
>   int s;
>  
> - s = splnet();
> + NET_LOCK(s);
>   LIST_REMOVE(sc, gif_list);
> - splx(s);
> + NET_UNLOCK(s);
>  
>   if_detach(ifp);
>  
> @@ -323,7 +323,6 @@ gif_ioctl(struct ifnet *ifp, u_long cmd,
>   int error = 0, size;
>   struct sockaddr *dst, *src;
>   struct sockaddr *sa;
> - int s;
>   struct gif_softc *sc2;
>  
>   switch (cmd) {
> @@ -466,10 +465,8 @@ gif_ioctl(struct ifnet *ifp, u_long cmd,
>   bcopy((caddr_t)dst, (caddr_t)sa, dst->sa_len);
>   sc->gif_pdst = sa;
>  
> - s = splnet();
>   ifp->if_flags |= IFF_RUNNING;
>   if_up(ifp); /* send up RTM_IFINFO */
> - splx(s);
>  
>   error = 0;
>   break;

Re: struct mbuf revisited

[Theo de Raadt]

That's OK with me.  Maybe the __LP64__ will be annoying enough that someone
does a m_hdr / pkthdr cleanup anyways.

struct mbuf revisited

[Mark Kettenis]

Turns out the __aligned(8) solution has some unforeseen side effects:

 /usr/src/sys/dev/ic/re.c:1602: warning: ignoring alignment for stack  
allocated 'mh'

While that warning is probably false in the sense that we don't
actually need the 8-byte alignment that we're asking for, the compiler
rightly complains that we shouldn't do this.  I think that this leaves
us adding explicit padding as the only viable option.

This diff uses #ifndef __LP64__.  Alternative would be to use u_long
and take hit of an extra 8-bytes on 64-bit architectures.  That's what
Theo prefers to force the folks that care to redesign this properly...

This diff also adds a CTASSERT such that we catch any issues with the
size of struct mbuf in the future.

Index: sys/mbuf.h
===
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.227
diff -u -p -r1.227 mbuf.h
--- sys/mbuf.h  10 May 2017 21:41:27 -  1.227
+++ sys/mbuf.h  16 May 2017 14:51:15 -
@@ -86,7 +86,10 @@ struct m_hdr {
u_int   mh_len; /* amount of data in this mbuf */
short   mh_type;/* type of data in this mbuf */
u_short mh_flags;   /* flags; see below */
-} __aligned(8);
+#ifndef __LP64__
+   u_int   mh_pad; /* pad to 8-byte boundary */
+#endif
+};
 
 /* pf stuff */
 struct pf_state_key;
Index: kern/uipc_mbuf.c
===
RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v
retrieving revision 1.246
diff -u -p -r1.246 uipc_mbuf.c
--- kern/uipc_mbuf.c8 May 2017 15:47:49 -   1.246
+++ kern/uipc_mbuf.c16 May 2017 14:51:15 -
@@ -100,6 +100,8 @@
 #include 
 #endif /* NPF > 0 */
 
+CTASSERT(sizeof(struct mbuf) == MSIZE);
+
 /* mbuf stats */
 COUNTERS_BOOT_MEMORY(mbstat_boot, MBSTAT_COUNT);
 struct cpumem *mbstat = COUNTERS_BOOT_INITIALIZER(mbstat_boot);

Re: avoid clang warnings and signed underflow in adventure(6)

[Marc Espie]

On Tue, May 16, 2017 at 11:18:01AM +0200, Theo Buehler wrote:
> Part of adventure's interesting internal obfuscation scheme makes clang
> very unhappy. It spews 240+ warnings of this kind:
> 
> /usr/src/games/adventure/wizard.c:71:17: warning: implicit conversion from 
> 'int' to 'char' changes value from 143 to -113
>   [-Wconstant-conversion]
> strlcpy(magic, DECR(d,w,a,r,f), sizeof magic);
>^~~
> /usr/src/games/adventure/hdr.h:159:33: note: expanded from macro 'DECR'
> #define DECR(a,b,c,d,e) decr(*#a+'+',*#b+'-',*#c+'#',*#d+'&',*#e+'%')
> 
> To avoid both, the warning and the signed underflow in decr(), let's do
> this no-op the other way around: subtract in the DECR macro and add back
> in decr().
> 
> Note that decr() is never called directly, it's only used via the
> DECR macro.

After the fix I did in gcc, you can also shut down selected warnings
just by adding -Wno-constant-conversion

Will work in gcc4, patch for gcc3 still awaiting review :)

avoid clang warnings and signed underflow in adventure(6)

[Theo Buehler]

Part of adventure's interesting internal obfuscation scheme makes clang
very unhappy. It spews 240+ warnings of this kind:

/usr/src/games/adventure/wizard.c:71:17: warning: implicit conversion from 
'int' to 'char' changes value from 143 to -113
  [-Wconstant-conversion]
strlcpy(magic, DECR(d,w,a,r,f), sizeof magic);
   ^~~
/usr/src/games/adventure/hdr.h:159:33: note: expanded from macro 'DECR'
#define DECR(a,b,c,d,e) decr(*#a+'+',*#b+'-',*#c+'#',*#d+'&',*#e+'%')

To avoid both, the warning and the signed underflow in decr(), let's do
this no-op the other way around: subtract in the DECR macro and add back
in decr().

Note that decr() is never called directly, it's only used via the
DECR macro.

Index: hdr.h
===
RCS file: /var/cvs/src/games/adventure/hdr.h,v
retrieving revision 1.16
diff -u -p -r1.16 hdr.h
--- hdr.h   10 Apr 2017 13:56:16 -  1.16
+++ hdr.h   14 May 2017 16:36:49 -
@@ -156,4 +156,4 @@ int turns, lmwarn, iwest, knfloc, detail
 intdemo, limit;
 
 /* We need to get a little tricky to avoid strings */
-#define DECR(a,b,c,d,e) decr(*#a+'+',*#b+'-',*#c+'#',*#d+'&',*#e+'%')
+#define DECR(a,b,c,d,e) decr(*#a-'+',*#b-'-',*#c-'#',*#d-'&',*#e-'%')
Index: init.c
===
RCS file: /var/cvs/src/games/adventure/init.c,v
retrieving revision 1.15
diff -u -p -r1.15 init.c
--- init.c  8 Mar 2016 10:48:39 -   1.15
+++ init.c  14 May 2017 16:36:59 -
@@ -66,11 +66,11 @@ decr(char a, char b, char c, char d, cha
 {
static char buf[6];
 
-   buf[0] = a - '+';
-   buf[1] = b - '-';
-   buf[2] = c - '#';
-   buf[3] = d - '&';
-   buf[4] = e - '%';
+   buf[0] = a + '+';
+   buf[1] = b + '-';
+   buf[2] = c + '#';
+   buf[3] = d + '&';
+   buf[4] = e + '%';
buf[5] = 0;
return buf;
 }