Re: [PATCH 03/04] Switch OCF and IPsec over to the new AES

[Mike Belopuhov]

On Mon, Apr 24, 2017 at 04:39 +0200, Mike Belopuhov wrote:
> AES_Setkey takes key length in bytes rather than bits which makes
> it a bit simpler.
> 

The diff below will have to go right after since glxsb depends on
xform.c to do AES-192 and AES-256...

>From 25a725a4440bdac11a4860af59dae4f705a76b7b Mon Sep 17 00:00:00 2001
From: Mike Belopuhov 
Date: Sun, 9 Apr 2017 23:33:50 +0200
Subject: [PATCH] Switch glxsb(4), VIA padlock and AES-NI drivers over to the
 new AES

---
 sys/arch/amd64/amd64/aesni.c | 2 +-
 sys/arch/amd64/amd64/via.c   | 6 +++---
 sys/arch/i386/i386/via.c | 6 +++---
 sys/arch/i386/pci/glxsb.c| 4 ++--
 sys/crypto/aes.h | 2 ++
 5 files changed, 11 insertions(+), 9 deletions(-)

diff --git sys/arch/amd64/amd64/aesni.c sys/arch/amd64/amd64/aesni.c
index c7cb918184e..cd09198246f 100644
--- sys/arch/amd64/amd64/aesni.c
+++ sys/arch/amd64/amd64/aesni.c
@@ -26,11 +26,11 @@
 #include 
 #include 
 #include 
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 
 #include 
diff --git sys/arch/amd64/amd64/via.c sys/arch/amd64/amd64/via.c
index 2e43f1eaf78..e99944c9bdd 100644
--- sys/arch/amd64/amd64/via.c
+++ sys/arch/amd64/amd64/via.c
@@ -34,11 +34,11 @@
 #include 
 #include 
 
 #ifdef CRYPTO
 #include 
-#include 
+#include 
 #include 
 #include 
 #endif
 
 #include 
@@ -192,13 +192,13 @@ viac3_crypto_newsession(u_int32_t *sidp, struct cryptoini 
*cri)
 
ses->ses_klen = c->cri_klen;
ses->ses_cw0 = cw0;
 
/* Build expanded keys for both directions */
-   rijndaelKeySetupEnc(ses->ses_ekey, c->cri_key,
+   AES_KeySetup_Encrypt(ses->ses_ekey, c->cri_key,
c->cri_klen);
-   rijndaelKeySetupDec(ses->ses_dkey, c->cri_key,
+   AES_KeySetup_Decrypt(ses->ses_dkey, c->cri_key,
c->cri_klen);
for (i = 0; i < 4 * (AES_MAXROUNDS + 1); i++) {
ses->ses_ekey[i] = ntohl(ses->ses_ekey[i]);
ses->ses_dkey[i] = ntohl(ses->ses_dkey[i]);
}
diff --git sys/arch/i386/i386/via.c sys/arch/i386/i386/via.c
index 7ea3d357aa0..27fdd908af8 100644
--- sys/arch/i386/i386/via.c
+++ sys/arch/i386/i386/via.c
@@ -34,11 +34,11 @@
 #include 
 #include 
 
 #ifdef CRYPTO
 #include 
-#include 
+#include 
 #include 
 #include 
 #endif
 
 #include 
@@ -195,13 +195,13 @@ viac3_crypto_newsession(u_int32_t *sidp, struct cryptoini 
*cri)
 
ses->ses_klen = c->cri_klen;
ses->ses_cw0 = cw0;
 
/* Build expanded keys for both directions */
-   rijndaelKeySetupEnc(ses->ses_ekey, c->cri_key,
+   AES_KeySetup_Encrypt(ses->ses_ekey, c->cri_key,
c->cri_klen);
-   rijndaelKeySetupDec(ses->ses_dkey, c->cri_key,
+   AES_KeySetup_Decrypt(ses->ses_dkey, c->cri_key,
c->cri_klen);
for (i = 0; i < 4 * (AES_MAXROUNDS + 1); i++) {
ses->ses_ekey[i] = ntohl(ses->ses_ekey[i]);
ses->ses_dkey[i] = ntohl(ses->ses_dkey[i]);
}
diff --git sys/arch/i386/pci/glxsb.c sys/arch/i386/pci/glxsb.c
index c88088dfac6..86d2f8e9120 100644
--- sys/arch/i386/pci/glxsb.c
+++ sys/arch/i386/pci/glxsb.c
@@ -38,11 +38,11 @@
 #include 
 #include 
 
 #ifdef CRYPTO
 #include 
-#include 
+#include 
 #include 
 #include 
 #endif
 
 #define SB_GLD_MSR_CAP 0x58002000  /* RO - Capabilities */
@@ -404,11 +404,11 @@ glxsb_crypto_newsession(uint32_t *sidp, struct cryptoini 
*cri)
if (swd == NULL) {
glxsb_crypto_freesession(sesn);
return (ENOMEM);
}
ses->ses_swd_enc = swd;
-   txf = _xform_rijndael128;
+   txf = _xform_aes;
if (txf->ctxsize > 0) {
swd->sw_kschedule =
malloc(txf->ctxsize,
M_CRYPTO_DATA,
M_NOWAIT|M_ZERO);
diff --git sys/crypto/aes.h sys/crypto/aes.h
index f3885c773ef..a670a2b522c 100644
--- sys/crypto/aes.h
+++ sys/crypto/aes.h
@@ -24,10 +24,12 @@
  */
 
 #ifndef _AES_H_
 #define _AES_H_
 
+#define AES_MAXROUNDS  (14)
+
 typedef struct aes_ctx {
uint32_t sk[60];
uint32_t sk_exp[120];
 
unsigned num_rounds;
-- 
2.12.2

[PATCH 04/04] Sync GMAC and AES-CTR/-XTS regress tests with the updated

[Mike Belopuhov]

Small regress test fixups.

---
 regress/sys/crypto/aesctr/Makefile  | 2 +-
 regress/sys/crypto/aesctr/aesctr.c  | 8 +++-
 regress/sys/crypto/aesxts/Makefile  | 2 +-
 regress/sys/crypto/aesxts/aes_xts.c | 6 +++---
 regress/sys/crypto/gmac/Makefile| 2 +-
 regress/sys/crypto/gmac/gmac_test.c | 2 +-
 6 files changed, 10 insertions(+), 12 deletions(-)

diff --git regress/sys/crypto/aesctr/Makefile regress/sys/crypto/aesctr/Makefile
index b1417864a2e..6c3dc3f9145 100644
--- regress/sys/crypto/aesctr/Makefile
+++ regress/sys/crypto/aesctr/Makefile
@@ -18,11 +18,11 @@ CDIAGFLAGS+=-Wsign-compare
 CDIAGFLAGS+=   -Wshadow
 
 REGRESS_ROOT_TARGETS=  run-regress-${PROG}
 
 .PATH: ${DIR}/crypto
-SRCS+= cast.c ecb_enc.c ecb3_enc.c gmac.c rijndael.c set_key.c
+SRCS+= cast.c ecb_enc.c ecb3_enc.c gmac.c aes.c set_key.c
 SRCS+= chachapoly.c poly1305.c
 SRCS+= xform.c
 
 run-regress-${PROG}: ${PROG}
./${PROG}
diff --git regress/sys/crypto/aesctr/aesctr.c regress/sys/crypto/aesctr/aesctr.c
index a7107331a8d..9b73993bbab 100644
--- regress/sys/crypto/aesctr/aesctr.c
+++ regress/sys/crypto/aesctr/aesctr.c
@@ -15,11 +15,11 @@
  * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
@@ -149,13 +149,12 @@ explicit_bzero(void *b, size_t len)
 #define AESCTR_NONCESIZE   4
 #define AESCTR_IVSIZE  8
 #define AESCTR_BLOCKSIZE   16
 
 struct aes_ctr_ctx {
-   u_int32_t   ac_ek[4*(AES_MAXROUNDS + 1)];
+   AES_CTX ac_key;
u_int8_tac_block[AESCTR_BLOCKSIZE];
-   int ac_nr;
 };
 
 int  aes_ctr_setkey(void *, u_int8_t *, int);
 void aes_ctr_encrypt(caddr_t, u_int8_t *);
 void aes_ctr_decrypt(caddr_t, u_int8_t *);
@@ -180,11 +179,10 @@ docrypt(const unsigned char *key, size_t klen, const 
unsigned char *iv,
aes_ctr_crypt(, block);
bcopy(block, out, AESCTR_BLOCKSIZE);
out += AESCTR_BLOCKSIZE;
}
return 0;
-
 }
 
 static int
 match(unsigned char *a, unsigned char *b, size_t len)
 {
@@ -240,11 +238,11 @@ run(int num)
return (1);
}
if (docrypt(data[TST_KEY], length[TST_KEY],
data[TST_IV], data[TST_PLAIN], p,
length[TST_PLAIN], 0) < 0) {
-   warnx("crypt with /dev/crypto failed");
+   warnx("encryption failed");
goto done;
}
fail = !match(data[TST_CIPHER], p, len);
printf("%s test vector %d\n", fail ? "FAILED" : "OK", num);
 done:
diff --git regress/sys/crypto/aesxts/Makefile regress/sys/crypto/aesxts/Makefile
index 4e1040c536a..bd3bc595c8d 100644
--- regress/sys/crypto/aesxts/Makefile
+++ regress/sys/crypto/aesxts/Makefile
@@ -18,11 +18,11 @@ CDIAGFLAGS+=-Wsign-compare
 CDIAGFLAGS+=   -Wshadow
 
 REGRESS_TARGETS=   run-regress-${PROG}
 
 .PATH:  ${DIR}/crypto
-SRCS+= cast.c ecb_enc.c ecb3_enc.c gmac.c rijndael.c set_key.c
+SRCS+= cast.c ecb_enc.c ecb3_enc.c gmac.c aes.c set_key.c
 SRCS+= chachapoly.c poly1305.c
 SRCS+= xform.c
 
 run-regress-${PROG}: ${PROG}
./${PROG}
diff --git regress/sys/crypto/aesxts/aes_xts.c 
regress/sys/crypto/aesxts/aes_xts.c
index 77a64108498..543eaceec87 100644
--- regress/sys/crypto/aesxts/aes_xts.c
+++ regress/sys/crypto/aesxts/aes_xts.c
@@ -24,23 +24,23 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
 #include 
 
 #defineAES_XTS_BLOCKSIZE   16
 
 struct aes_xts_ctx {
-   rijndael_ctx key1;
-   rijndael_ctx key2;
+   AES_CTX key1;
+   AES_CTX key2;
u_int8_t tweak[AES_XTS_BLOCKSIZE];
 };
 
 int  aes_xts_setkey(void *, u_int8_t *, int);
 void aes_xts_encrypt(caddr_t, u_int8_t *);
diff --git regress/sys/crypto/gmac/Makefile regress/sys/crypto/gmac/Makefile
index 9e687f53e20..edb3c10d7c5 100644
--- regress/sys/crypto/gmac/Makefile
+++ regress/sys/crypto/gmac/Makefile
@@ -1,11 +1,11 @@
 #  $OpenBSD: Makefile,v 1.2 2014/01/18 05:54:52 martynas Exp $
 
 DIR=${.CURDIR}/../../../../sys
 
 PROG=  gmac_test
-SRCS+= rijndael.c gmac.c gmac_test.c
+SRCS+= aes.c gmac.c gmac_test.c
 CDIAGFLAGS=-Wall
 CDIAGFLAGS+=   -Werror
 CDIAGFLAGS+=   -Wpointer-arith
 CDIAGFLAGS+=   -Wno-uninitialized
 CDIAGFLAGS+=   -Wstrict-prototypes
diff --git regress/sys/crypto/gmac/gmac_test.c 
regress/sys/crypto/gmac/gmac_test.c
index 90732976d34..8717de81cb1 100644
--- regress/sys/crypto/gmac/gmac_test.c
+++ regress/sys/crypto/gmac/gmac_test.c
@@ -16,11 +16,11 @@
  * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
 #include 
-#include 
+#include 
 #include 

[PATCH 03/04] Switch OCF and IPsec over to the new AES

[Mike Belopuhov]

AES_Setkey takes key length in bytes rather than bits which makes
it a bit simpler.

---
 sys/crypto/cryptosoft.c |  8 +++
 sys/crypto/gmac.c   |  9 
 sys/crypto/gmac.h   |  5 ++---
 sys/crypto/xform.c  | 55 +++--
 sys/crypto/xform.h  |  2 +-
 sys/netinet/ip_esp.c|  2 +-
 6 files changed, 38 insertions(+), 43 deletions(-)

diff --git sys/crypto/cryptosoft.c sys/crypto/cryptosoft.c
index 315210c5407..bdc5602f6fe 100644
--- sys/crypto/cryptosoft.c
+++ sys/crypto/cryptosoft.c
@@ -793,12 +793,12 @@ swcr_newsession(u_int32_t *sid, struct cryptoini *cri)
txf = _xform_blf;
goto enccommon;
case CRYPTO_CAST_CBC:
txf = _xform_cast5;
goto enccommon;
-   case CRYPTO_RIJNDAEL128_CBC:
-   txf = _xform_rijndael128;
+   case CRYPTO_AES_CBC:
+   txf = _xform_aes;
goto enccommon;
case CRYPTO_AES_CTR:
txf = _xform_aes_ctr;
goto enccommon;
case CRYPTO_AES_XTS:
@@ -958,11 +958,11 @@ swcr_freesession(u_int64_t tid)
 
switch (swd->sw_alg) {
case CRYPTO_3DES_CBC:
case CRYPTO_BLF_CBC:
case CRYPTO_CAST_CBC:
-   case CRYPTO_RIJNDAEL128_CBC:
+   case CRYPTO_AES_CBC:
case CRYPTO_AES_CTR:
case CRYPTO_AES_XTS:
case CRYPTO_AES_GCM_16:
case CRYPTO_AES_GMAC:
case CRYPTO_CHACHA20_POLY1305:
@@ -1142,11 +1142,11 @@ swcr_init(void)
algs[CRYPTO_BLF_CBC] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_CAST_CBC] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_MD5_HMAC] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_SHA1_HMAC] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_RIPEMD160_HMAC] = CRYPTO_ALG_FLAG_SUPPORTED;
-   algs[CRYPTO_RIJNDAEL128_CBC] = CRYPTO_ALG_FLAG_SUPPORTED;
+   algs[CRYPTO_AES_CBC] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_AES_CTR] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_AES_XTS] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_AES_GCM_16] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_AES_GMAC] = CRYPTO_ALG_FLAG_SUPPORTED;
algs[CRYPTO_DEFLATE_COMP] = CRYPTO_ALG_FLAG_SUPPORTED;
diff --git sys/crypto/gmac.c sys/crypto/gmac.c
index 33843d08fbe..765f8404178 100644
--- sys/crypto/gmac.c
+++ sys/crypto/gmac.c
@@ -23,11 +23,11 @@
  */
 
 #include 
 #include 
 
-#include 
+#include 
 #include 
 
 void   ghash_gfmul(uint32_t *, uint32_t *, uint32_t *);
 void   ghash_update_mi(GHASH_CTX *, uint8_t *, size_t);
 
@@ -112,16 +112,15 @@ AES_GMAC_Init(void *xctx)
 void
 AES_GMAC_Setkey(void *xctx, const uint8_t *key, uint16_t klen)
 {
AES_GMAC_CTX*ctx = xctx;
 
-   ctx->rounds = rijndaelKeySetupEnc(ctx->K, (u_char *)key,
-   (klen - AESCTR_NONCESIZE) * 8);
+   AES_Setkey(>K, key, klen - AESCTR_NONCESIZE);
/* copy out salt to the counter block */
bcopy(key + klen - AESCTR_NONCESIZE, ctx->J, AESCTR_NONCESIZE);
/* prepare a hash subkey */
-   rijndaelEncrypt(ctx->K, ctx->rounds, ctx->ghash.H, ctx->ghash.H);
+   AES_Encrypt(>K, ctx->ghash.H, ctx->ghash.H);
 }
 
 void
 AES_GMAC_Reinit(void *xctx, const uint8_t *iv, uint16_t ivlen)
 {
@@ -160,10 +159,10 @@ AES_GMAC_Final(uint8_t digest[GMAC_DIGEST_LEN], void 
*xctx)
uint8_t keystream[GMAC_BLOCK_LEN];
int i;
 
/* do one round of GCTR */
ctx->J[GMAC_BLOCK_LEN - 1] = 1;
-   rijndaelEncrypt(ctx->K, ctx->rounds, ctx->J, keystream);
+   AES_Encrypt(>K, ctx->J, keystream);
for (i = 0; i < GMAC_DIGEST_LEN; i++)
digest[i] = ctx->ghash.S[i] ^ keystream[i];
explicit_bzero(keystream, sizeof(keystream));
 }
diff --git sys/crypto/gmac.h sys/crypto/gmac.h
index bae2c80864d..63e1988701e 100644
--- sys/crypto/gmac.h
+++ sys/crypto/gmac.h
@@ -17,11 +17,11 @@
  */
 
 #ifndef _GMAC_H_
 #define _GMAC_H_
 
-#include 
+#include 
 
 #define GMAC_BLOCK_LEN 16
 #define GMAC_DIGEST_LEN16
 
 typedef struct _GHASH_CTX {
@@ -30,13 +30,12 @@ typedef struct _GHASH_CTX {
uint8_t Z[GMAC_BLOCK_LEN];  /* initial state */
 } GHASH_CTX;
 
 typedef struct _AES_GMAC_CTX {
GHASH_CTX   ghash;
-   uint32_tK[4*(AES_MAXROUNDS + 1)];
+   AES_CTX K;
uint8_t J[GMAC_BLOCK_LEN];  /* counter block */
-   int rounds;
 } AES_GMAC_CTX;
 
 __BEGIN_DECLS
 extern void (*ghash_update)(GHASH_CTX *, uint8_t *, size_t);
 
diff --git sys/crypto/xform.c sys/crypto/xform.c
index 0a4ddbb3ffd..70d460f3f58 100644
--- sys/crypto/xform.c
+++ sys/crypto/xform.c
@@ -57,11 +57,11 @@
 #include 
 #include 
 #include 
 #include 
 

[PATCH 02/04] Adjust AES testcase to the new implementation

[Mike Belopuhov]

Adjusts the regress test.

---
 regress/sys/crypto/aes/Makefile  |  2 +-
 regress/sys/crypto/aes/aestest.c | 10 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git regress/sys/crypto/aes/Makefile regress/sys/crypto/aes/Makefile
index 75e88b1a7a6..9120371a07d 100644
--- regress/sys/crypto/aes/Makefile
+++ regress/sys/crypto/aes/Makefile
@@ -17,11 +17,11 @@ CDIAGFLAGS+=-Wsign-compare
 CDIAGFLAGS+=   -Wshadow
 
 REGRESS_ROOT_TARGETS=  run-regress-${PROG}
 
 .PATH: ${DIR}/crypto
-SRCS+= rijndael.c
+SRCS+= aes.c
 
 run-regress-${PROG}: ${PROG}
./${PROG} ${.CURDIR}/vectors/*.txt
 
 .include 
diff --git regress/sys/crypto/aes/aestest.c regress/sys/crypto/aes/aestest.c
index f51be2a2665..489ac404e45 100644
--- regress/sys/crypto/aes/aestest.c
+++ regress/sys/crypto/aes/aestest.c
@@ -29,11 +29,11 @@
  * Test kernel AES implementation with test vectors provided by
  * Dr Brian Gladman:  http://fp.gladman.plus.com/AES/
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
@@ -41,21 +41,21 @@
 
 static int
 docrypt(const unsigned char *key, size_t klen, const unsigned char *in,
 unsigned char *out, size_t len, int do_encrypt)
 {
-   rijndael_ctx ctx;
+   AES_CTX ctx;
int error = 0;
 
memset(, 0, sizeof(ctx));
-   error = rijndael_set_key(, key, klen * 8);
+   error = AES_Setkey(, key, klen);
if (error)
return -1;
if (do_encrypt)
-   rijndael_encrypt(, in, out);
+   AES_Encrypt(, in, out);
else
-   rijndael_decrypt(, in, out);
+   AES_Decrypt(, in, out);
return 0;
 }
 
 static int
 match(unsigned char *a, unsigned char *b, size_t len)
-- 
2.12.2

[PATCH 01/04] Constant time AES implementation

[Mike Belopuhov]

Hi,

This is the first diff in series to replace our table-driven AES
implementation in the crypto framework with a constant time one
authored by Thomas Pornin.  I've been on the lookout for a complete
constant time implementation for several years and thanks to Thomas,
we've got a very nice one now.

I've started fitting this code into our framework and exploring the
rijndael.c dependencies around last Christmas, but wasn't able to
get back to it until now.  In my github branch newaes2[1] I've got a
complete set of diffs for those who want to go look ahead, but I
believe it's time to start getting it slowly into the tree.

I've got a few positive test reports, however until today, the IPsec
was broken because of a somewhat tricky change[2].  Now it should be
fixed.

This change introduces the 32-bit constant time AES implementation
from BearSSL[3] merging aes_ct.c, aes_ct_dec.c and aes_ct_enc.c.
Thomas was kind enough to review my wrapping code and has optimized
and largely rewritten it exposing additional API for NIST-compatible
subkey expansion required (in OpenBSD) by the VIA padlock driver:
AES_KeySetup_Encrypt and AES_KeySetup_Decrypt.

Initially, I've considered using both ct and ct64 implementations but,
as Thomas has rightfully pointed out, ct64 is faster when multiple
blocks are processed at the same time which is never the case in our
framework at the moment and direct calls to AES usually implement some
kind of a cipher mode and thus are not able to take advantage of it as
well.  Therefore aes_ct64.c was dropped and aes_ct.c was included
directly into the aes.c.

OK?

[1] https://github.com/mbelop/src/commits/newaes2
[2] 
https://github.com/mbelop/src/commit/79c082e9fbe2decda63f782e4fd77fd4a722831d
[3] https://bearssl.org/

---
 sys/conf/files   |   1 +
 sys/crypto/aes.c | 877 +++
 sys/crypto/aes.h |  45 +++
 3 files changed, 923 insertions(+)
 create mode 100644 sys/crypto/aes.c
 create mode 100644 sys/crypto/aes.h

diff --git sys/conf/files sys/conf/files
index cc86def23f2..26737748201 100644
--- sys/conf/files
+++ sys/conf/files
@@ -848,10 +848,11 @@ file netinet/ipsec_input.cipsec
 file netinet/ipsec_output.cipsec
 file netinet/ip_esp.c  ipsec
 file netinet/ip_ah.c   ipsec
 file netinet/ip_carp.c carpneeds-count
 file netinet/ip_ipcomp.c   ipsec
+file crypto/aes.c  ipsec | crypto | uvm_swap_encrypt | wlan
 file crypto/rijndael.c ipsec | crypto | uvm_swap_encrypt | wlan
 file crypto/md5.c
 file crypto/rmd160.c   ipsec | crypto
 file crypto/sha1.c ipsec | crypto | carp | wlan
 file crypto/sha2.c
diff --git sys/crypto/aes.c sys/crypto/aes.c
new file mode 100644
index 000..e4feaf74636
--- /dev/null
+++ sys/crypto/aes.c
@@ -0,0 +1,877 @@
+/*
+ * Copyright (c) 2016 Thomas Pornin 
+ *
+ * Modified for OpenBSD by Thomas Pornin and Mike Belopuhov.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+
+#include "aes.h"
+
+static inline void
+enc32le(void *dst, uint32_t x)
+{
+   unsigned char *buf = dst;
+
+   buf[0] = (unsigned char)x;
+   buf[1] = (unsigned char)(x >> 8);
+   buf[2] = (unsigned char)(x >> 16);
+   buf[3] = (unsigned char)(x >> 24);
+}
+
+static inline uint32_t
+dec32le(const void *src)
+{
+   const unsigned char *buf = src;
+
+   return (uint32_t)buf[0]
+   | ((uint32_t)buf[1] << 8)
+   | ((uint32_t)buf[2] << 16)
+   | ((uint32_t)buf[3] << 24);
+}
+
+/*
+ * This constant-time implementation is "bitsliced": the 128-bit state is
+ * split over eight 32-bit words q* in the following way:
+ *
+ * -- Input block consists in 16 bytes:
+ *a00 a10 a20 a30 a01 a11 a21 a31 a02 a12 a22 a32 a03 a13 a23 

Re: snprintf(3) example warns under -Wextra

[Timo Buhrmester]

On Mon, Apr 24, 2017 at 12:01:10AM +0200, Marc Espie wrote:
> On Sun, Apr 23, 2017 at 10:16:38PM +0200, Timo Buhrmester wrote:
> > > The main difference between you and Theo is that Theo knows what he's
> > > talking about.
> 
> > If you want to contribute anything, point out how casting a non-negative
> > into to size_t for comparison against another size_t could lead to 
> > "real errors later whenever the code evolves in any way".  Nice straw-man
> > with the s/world/code/, too.  Sure makes it easier to appear to have a 
> > point.
> 

You are talking about the code in question changing, I am talking about
the environment around it changing.  While I agree with you on a rule-of-thumb
basis where casts are concerned, I don't see it as much as a blanket rule
as you seem to do.  Reminds me of people who will religiously try to spread
the word that goto has no uses and must be avoided at all costs.

> Say, somebody renames variables, and boom, after a few changes, you are
> casting a pointer into a size_t.
That's an odd thing to happen by renaming only.

If the premise is that code can be broken by messing with it, that's
trivially true and nothing you need to convince me of.  In case you have
missed it, the whole "changing the world" thing started as:
> Unneccesary casts were among the hardest parts of the jump from 32-bit
> to 64-bit, since a cast means "I know better than the compiler".
> Well, when the world changes, that cast is suddenly wrong.

And all I pointed out that the cast I proposed is safe from such changes
in architecture, or new C standards, etc.  And I still stand by that claim.

Besides, I pointed out a real potential issue with the code that is
completely independent of whether or not there is a cast, yet that's
being ignored.

This is my last reply in this pointless thread.

Re: snprintf(3) example warns under -Wextra

[Marc Espie]

On Sun, Apr 23, 2017 at 10:16:38PM +0200, Timo Buhrmester wrote:
> > The main difference between you and Theo is that Theo knows what he's
> > talking about.

> If you want to contribute anything, point out how casting a non-negative
> into to size_t for comparison against another size_t could lead to 
> "real errors later whenever the code evolves in any way".  Nice straw-man
> with the s/world/code/, too.  Sure makes it easier to appear to have a point.

You definitely don't get it, do you ? people routinely change code in
sweeping ways, and rely on the compiler to catch a lot of small mistakes.

Say, somebody renames variables, and boom, after a few changes, you are
casting a pointer into a size_t.  Guess what ? you added a cast, so compiler
doesn't warn (yes, I just checked, neither gcc nor clang warn on THAT one)

I've seen bogus code countless times because a cast hid an horrid mistake.

When you review code for mistakes and bugs, every time you see a cast,
you have to check the variables, verify something isn't amiss. Because the
compiler won't warn you.

I have seen this countless times over the years.   Like old X code playing
mixing pointers and integers, and suddenly no longer working on 64 bit 
machines.

Or somewhat more fun code using that old T *p = (T*)malloc(sz);  idiom...
Until there's T* on one side, and something else on the other side.

Bottom line, each time, I read a cast, I have to slow down, and check it
manually. I know the programmer did tell the compiler to explicitly
SHUT THE FUCK UP, so the seat belts are GONE, not even detached, but 
completely BURNT THRU WITH NAPALM.

*Whenever* I can avoid a cast, I do.

Re: snprintf(3) example warns under -Wextra

[Timo Buhrmester]

> The main difference between you and Theo is that Theo knows what he's
> talking about.
If you want to contribute anything, point out how casting a non-negative
into to size_t for comparison against another size_t could lead to 
"real errors later whenever the code evolves in any way".  Nice straw-man
with the s/world/code/, too.  Sure makes it easier to appear to have a point.

Re: snprintf(3) example warns under -Wextra

[Marc Espie]

On Sun, Apr 23, 2017 at 05:12:16PM +0200, Timo Buhrmester wrote:
> Except if the world changes...  In a way that's still POSIX-compliant.
> But why would anyone want to protect themselves from that, right?

You're full of it.

You're advocating for an unnecessary cast that can actually hide real
errors later whenever the code evolves in any way, and now you say
that the world can change.

The main difference between you and Theo is that Theo knows what he's
talking about.

Unnecessary casts to silence stupid warnings have always been a plague
that leads to large bugs and big security issues.

Re: Ldomctl interrupt boot

[Andrew Grillet]

Hi Stefan,

Thanks for your help.

It seems there was a hardware problem. I was running in SSH, and that did
not work (I knew it had previously in other situations).
In the end, I had to power down completely - including removing the cables
from the PSUs!

After the reboot, it behaved as you described.

On 23 April 2017 at 15:45, Stefan Sperling  wrote:

> On Sun, Apr 23, 2017 at 02:14:51PM +0100, Andrew Grillet wrote:
> > Hi
> >
> > I have a T2000 running OpenBSD 6.0, with five guest domains.
> >
> > Primary and some guests are working. I am attempting to install the OS in
> > the remaining guests. In the process of installing the OS on vdisk0 of a
> > guest, I wish it to boot from vdisk1, but the guest is stuck in a
> situation
> > where it continually attempts to netboot.
> >
> > I am at the console using cu -l ttyVx.
> >
> > Is there a way to interrupt the continuous netbooting attempts? it does
> not
> > respond to anything I can find on the keyboard. Most commands are
> > intercepted by the primary domain.
>
> A break should interupt it and bring you to the ok> prompt.
> Type this to cu:
>
>   ~#
>
> If cu is running in SSH, you need to escape the tilde with
> another tilde:
>
>   ~~#
>

uaudio_drain() not needed?

[Michael W. Bombardieri]

Hi,

When I remove uaudio_drain() on my laptop the attach/detach still
seems to work as expected.
I did a test with two usb soundcards. Audio files were played &
recorded using aucat.

* boot system (no audio device because I disabled azalia)
* attach device1 (Creative card)
* play wav file1 (reference file)
* detach device1
* attach device2 (Yamaha card)
* play wav file1
* record wav file2
* detach device2
* attach device2
* play wav file2
* detach device2
* attach device1
* play wav file2
* detach device1

So far this has only been tested on amd64. Maybe it produces
issues for your uaudio device though.

- Michael


Index: uaudio.c
===
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.126
diff -u -p -u -r1.126 uaudio.c
--- uaudio.c8 Apr 2017 02:57:25 -   1.126
+++ uaudio.c23 Apr 2017 15:34:53 -
@@ -371,7 +371,6 @@ voiduaudio_chan_rintr
 
 intuaudio_open(void *, int);
 void   uaudio_close(void *);
-intuaudio_drain(void *);
 void   uaudio_get_minmax_rates
(int, const struct as_info *, const struct audio_params *,
 int, int, int, u_long *, u_long *);
@@ -563,7 +562,6 @@ int
 uaudio_detach(struct device *self, int flags)
 {
struct uaudio_softc *sc = (struct uaudio_softc *)self;
-   int rv = 0;
 
/*
 * sc_alts may be NULL if uaudio_identify_as() failed, in
@@ -571,14 +569,8 @@ uaudio_detach(struct device *self, int f
 * nothing to detach.
 */
if (sc->sc_alts == NULL)
-   return (rv);
-
-   /* Wait for outstanding requests to complete. */
-   uaudio_drain(sc);
-
-   rv = config_detach_children(self, flags);
-
-   return (rv);
+   return (0);
+   return (config_detach_children(self, flags));
 }
 
 const usb_interface_descriptor_t *
@@ -2107,24 +2099,6 @@ uaudio_close(void *addr)
uaudio_chan_close(sc, >sc_playchan);
if (sc->sc_recchan.altidx != -1)
uaudio_chan_close(sc, >sc_recchan);
-}
-
-int
-uaudio_drain(void *addr)
-{
-   struct uaudio_softc *sc = addr;
-   struct chan *pchan = >sc_playchan;
-   struct chan *rchan = >sc_recchan;
-   int ms = 0;
-
-   /* Wait for outstanding requests to complete. */
-   if (pchan->altidx != -1 && sc->sc_alts[pchan->altidx].sc_busy)
-   ms = max(ms, pchan->reqms);
-   if (rchan->altidx != -1 && sc->sc_alts[rchan->altidx].sc_busy)
-   ms = max(ms, rchan->reqms);
-   usbd_delay_ms(sc->sc_udev, UAUDIO_NCHANBUFS * ms);
-
-   return (0);
 }
 
 int

Re: armv7/omap: introducing amdisplay(4) + nxptda(4)

[Mark Kettenis]

> Date: Mon, 3 Apr 2017 01:33:28 -0400
> From: Ian Sutton 
> 
> This patch introduces preliminary support for the display hardware
> onboard the am335x/BeagleBone Black. Included are two drivers,
> amdisplay(4) and nxptda(4), which run the LCD controller and HDMI PHY
> transmitter respectively. This patch follows work I've mailed to this
> list found here:
> 
> https://marc.info/?t=14704720461=1=2
> 
> The code below is a complete (fourth!) rewrite. It completely and
> correctly controls the hardware but still needs to be plugged into the
> rest of the system (wsdisplay for syscons, wsfb for Xorg, etc.) before
> it is useful. In the meantime, I feel that it is review-ready and worth
> the time for a dev to look over.
> 
> Here is a demonstration of my code working where the framebuffer is
> filled with entropy from arc4random_buf() every frame:
> 
> https://www.youtube.com/watch?v=yCzFyZlfbzQ
> 
> Here is the man page in HTML format for convenience:
> 
> https://ce.gl/amdisplay.4.html
> 
> Here is a link to the patch below for when the list mangles it:
> 
> https://ce.gl/amdisplay-apr2.diff.txt
> 
> Note: this driver currently only supports 640x480x16 mode, but is only
> one clock/math problem away from higher modes. Yes, I am aware of fdt
> simple-framebuffer node and yes, this code accomplishes useful things
> which the fdt framebuffer cannot.

Ian,

As you've noticed this diff hasn't seen any real review yet.  One of
the problems is that it adds support for something we don't really
consider to be very important for armv7, on hardware that most of us
consider to be mostly a (very slow) toy which is hard to actually use
as having a framebuffer doesn't really make sense as long as the
serial console is the only actual input device that works in OpenBSD
on the BBB.

The diff itself looks mostly reasonable.  It's probably better to
rename nxptda(4) into something like nxphdmi(4), as nxptda(4) isn't a
very descriptive name.  But before we ask you to make such changes, I
think we really need working USB support.  Without that, this code
really isn't very useful.

Mark

> Index: sys/arch/armv7/conf/GENERIC
> ===
> RCS file: /cvs/src/sys/arch/armv7/conf/GENERIC,v
> retrieving revision 1.71
> diff -u -p -p -u -r1.71 GENERIC
> --- sys/arch/armv7/conf/GENERIC   23 Jan 2017 22:43:17 -  1.71
> +++ sys/arch/armv7/conf/GENERIC   3 Apr 2017 05:24:32 -
> @@ -81,6 +81,9 @@ sdmmc*  at ommmc?   # SD/MMC bus
>  
>  omehci*  at fdt? # EHCI
>  usb* at omehci?
> +nxptda*  at iic? # TDA19988 HDMI PHY
> +amdisplay*   at fdt? # AM335x LCD controller
> +wsdisplay*   at amdisplay?
>  
>  # Sunxi A1x/A20 SoC
>  sxiintc* at fdt? # A1x interrupt controller
> Index: sys/arch/armv7/omap/am335x_prcmreg.h
> ===
> RCS file: /cvs/src/sys/arch/armv7/omap/am335x_prcmreg.h,v
> retrieving revision 1.4
> diff -u -p -p -u -r1.4 am335x_prcmreg.h
> --- sys/arch/armv7/omap/am335x_prcmreg.h  18 Mar 2014 07:34:17 -  
> 1.4
> +++ sys/arch/armv7/omap/am335x_prcmreg.h  3 Apr 2017 05:24:33 -
> @@ -20,6 +20,7 @@
>  #define AM335X_CLKCTRL_MODULEMODE_MASK   0x0003
>  
>  #define PRCM_AM335X_CM_PER   0x
> +#define PRCM_AM335X_LCDC_CLKCTRL 0x0018
>  #define PRCM_AM335X_USB0_CLKCTRL 0x001c
>  #define PRCM_AM335X_TPTC0_CLKCTRL0x0024
>  #define PRCM_AM335X_MMC0_CLKCTRL 0x003c
> @@ -38,6 +39,10 @@
>  #define PRCM_AM335X_CM_WKUP  0x0400
>  #define PRCM_AM335X_GPIO0_CLKCTRL0x0408
>  #define PRCM_AM335X_TIMER0_CLKCTRL   0x0410
> +#define PRCM_AM335X_DISP_IDLEST  0x0448
> +#define PRCM_AM335X_DISP_CLKSEL  0x0454
> +#define PRCM_AM335X_DISP_CLKMODE 0x0498
> +#define PRCM_AM335X_DISP_M2  0x04a4
>  #define PRCM_AM335X_I2C0_CLKCTRL 0x04b8
>  #define PRCM_AM335X_CM_DPLL  0x0500
>  #define PRCM_AM335X_CLKSEL_TIMER2_CLK0x0508
> Index: sys/arch/armv7/omap/amdisplay.c
> ===
> RCS file: sys/arch/armv7/omap/amdisplay.c
> diff -N sys/arch/armv7/omap/amdisplay.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ sys/arch/armv7/omap/amdisplay.c   3 Apr 2017 05:24:33 -
> @@ -0,0 +1,628 @@
> +/*
> + * Copyright (c) 2016 Ian Sutton 
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> 

Re: xinit(1): xdm(1) -> xenodm(1)

[Jason McIntyre]

On Sun, Apr 23, 2017 at 10:23:14AM -0500, Scott Cheloha wrote:
> 
> > On Apr 23, 2017, at 2:44 AM, Jason McIntyre  wrote:
> > 
> > [...] but note that all this text is doing is giving an example of a
> > window manager and, as such, it's not wrong.
> 
> My thinking is that preventing stuff like
> 
>   $ man xdm
>   man: No entry for xdm in the manual.
> 
> where possible is probably a good thing.
> 
> Given the choice, why not reference the thing documented within the system?
> Especially when it's a neutral thing, as you point out, like providing an
> example of such a piece of software.
> 
> -Scott
> 

well because X docs are third party, and we don't usually make local
changes to third party docs. having said that, there are exceptions,
and i know we have changed some docs (forget which) to mention
xenodm. so that's why i said i'd leave it to the X folks ;)

jmc

Re: snprintf(3) example warns under -Wextra

[Timo Buhrmester]

> Timo if you feel the need to be an asshole, please be that asshole
> elsewhere.
Pot, meet Kettle.

Re: xinit(1): xdm(1) -> xenodm(1)

[Scott Cheloha]

> On Apr 23, 2017, at 2:44 AM, Jason McIntyre  wrote:
> 
> [...] but note that all this text is doing is giving an example of a
> window manager and, as such, it's not wrong.

My thinking is that preventing stuff like

$ man xdm
man: No entry for xdm in the manual.

where possible is probably a good thing.

Given the choice, why not reference the thing documented within the system?
Especially when it's a neutral thing, as you point out, like providing an
example of such a piece of software.

-Scott

Re: snprintf(3) example warns under -Wextra

[Theo de Raadt]

> > Otherwise, you can start enabling that option and sending a diff which
> > fixes ALL THE WARNINGS IT GIVES IN THE ENTIRE TREE.
> I think I'll pass on that.  I wasn't aware of how many warnings
> a build seems to cause.  This must be why NetBSD has -Werror in their
> CFLAGS.

Timo if you feel the need to be an asshole, please be that asshole
elsewhere.

Re: snprintf(3) example warns under -Wextra

[Timo Buhrmester]

> > Otherwise people might fall into a habit
> > of ignoring warnings [that may point to actual problems].
> 
> People might fall into the habit of ignoring a warning from an
> extension to C provided by a single compiler?
> 
> I doubt it.
Doubt away.  I find it more than obvious that telling people
to ignore some warnings will make them more likely to also ignore
and underestimate other warnings.  Provided they even notice
the serious warnings in a stream of harmless warnings.  Whatever.


> There is no point in silencing the warning, since the warning is
> from an extension to C which is bullshit.
What extension of C are you talking about?


> > (*) looking at POSIX, snprintf is not required to return -1,
> > but only "a negative value".  So it's not truly safe either way.
> 
> It cannot occur in this code in any case.
Except if the world changes...  In a way that's still POSIX-compliant.
But why would anyone want to protect themselves from that, right?


> You are barking up the wrong tree.
Obviously I am.


> Otherwise, you can start enabling that option and sending a diff which
> fixes ALL THE WARNINGS IT GIVES IN THE ENTIRE TREE.
I think I'll pass on that.  I wasn't aware of how many warnings
a build seems to cause.  This must be why NetBSD has -Werror in their
CFLAGS.

Re: snprintf(3) example warns under -Wextra

[Theo de Raadt]

> > The code is already safe.
> It is reasonably safe(*) and triggers a warning. That's a good reason
> to silence the warning.

No.

The warning is a false extension to C.

In C, int and sizeof can be compared safely in this circumstance.

> Otherwise people might fall into a habit
> of ignoring warnings [that may point to actual problems].

People might fall into the habit of ignoring a warning from an
extension to C provided by a single compiler?

I doubt it.

> I just pointed out a safe way to silence the warning, without it
> potentially blowing up in a changed world.

There is no point in silencing the warning, since the warning is
from an extension to C which is bullshit.

> (*) looking at POSIX, snprintf is not required to return -1,
> but only "a negative value".  So it's not truly safe either way.

It cannot occur in this code in any case.


You are barking up the wrong tree.

Otherwise, you can start enabling that option and sending a diff which
fixes ALL THE WARNINGS IT GIVES IN THE ENTIRE TREE.

Which we will gladly delete also.

Because the tree is written in C, and C allows that idiom, because it
is safe.

Re: Ldomctl interrupt boot

[Stefan Sperling]

On Sun, Apr 23, 2017 at 02:14:51PM +0100, Andrew Grillet wrote:
> Hi
> 
> I have a T2000 running OpenBSD 6.0, with five guest domains.
> 
> Primary and some guests are working. I am attempting to install the OS in
> the remaining guests. In the process of installing the OS on vdisk0 of a
> guest, I wish it to boot from vdisk1, but the guest is stuck in a situation
> where it continually attempts to netboot.
> 
> I am at the console using cu -l ttyVx.
> 
> Is there a way to interrupt the continuous netbooting attempts? it does not
> respond to anything I can find on the keyboard. Most commands are
> intercepted by the primary domain.

A break should interupt it and bring you to the ok> prompt.
Type this to cu:

  ~#

If cu is running in SSH, you need to escape the tilde with
another tilde:

  ~~#

Re: snprintf(3) example warns under -Wextra

[Timo Buhrmester]

> The code is already safe.
It is reasonably safe(*) and triggers a warning. That's a good reason
to silence the warning.  Otherwise people might fall into a habit
of ignoring warnings [that may point to actual problems].  I just
pointed out a safe way to silence the warning, without it
potentially blowing up in a changed world.

(*) looking at POSIX, snprintf is not required to return -1,
but only "a negative value".  So it's not truly safe either way.

Re: snprintf(3) example warns under -Wextra

[Theo de Raadt]

> > Well, when the world changes, that cast is suddenly wrong.
> 
> I.e. instead of
> > if (ret == -1 || ret >= sizeof(buffer))
> one could use
> > if (ret < 0 || (size_t)ret >= sizeof(buffer))
> 
> And be safe from a changing world.


The code is already safe.

Re: snprintf(3) example warns under -Wextra

[Timo Buhrmester]

> Well, when the world changes, that cast is suddenly wrong.

I.e. instead of
>   if (ret == -1 || ret >= sizeof(buffer))
one could use
>   if (ret < 0 || (size_t)ret >= sizeof(buffer))

And be safe from a changing world.

Ldomctl interrupt boot

[Andrew Grillet]

Hi

I have a T2000 running OpenBSD 6.0, with five guest domains.

Primary and some guests are working. I am attempting to install the OS in
the remaining guests. In the process of installing the OS on vdisk0 of a
guest, I wish it to boot from vdisk1, but the guest is stuck in a situation
where it continually attempts to netboot.

I am at the console using cu -l ttyVx.

Is there a way to interrupt the continuous netbooting attempts? it does not
respond to anything I can find on the keyboard. Most commands are
intercepted by the primary domain.

Ideally, I would like to be able to force auto-boot? to false from sc or
the primary using eeprom or ldomctl. Failing that, just forcing it back to
the ok prompt would be good. I really do not want to have to implement
netbooting.

This must be a general problem, so I assume there is a solution other than
netbooting, but if not, surely there needs to be one!

regards

Andrew

Re: pfctl(8): reduce -k

[Jason McIntyre]

On Sun, Apr 23, 2017 at 12:41:37PM +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> I'm fine with your pfctl.c change.
> 
> Although I like your brief version of manpage, wearing admin's hat, I'm
> somewhat missing line:
> > -k host | network | label | key | id
> 
> so how about one small change below:
> 
> 8<---8<---8<--8<
> --- pfctl.8.new.jason Sun Apr 23 12:10:00 2017
> +++ pfctl.8.sashanSun Apr 23 12:15:56 2017
> @@ -224,7 +224,7 @@
>  .Fl K
>  option may be specified, which will kill all the source tracking entries
>  from the first host/network to the second.
> -.It Fl k Ar key
> +.It Fl k Ar host | network
>  Kill all of the state entries originating from the
>  host or network specified by
>  .Ar key .
> @@ -239,6 +239,7 @@
>  .Pp
>  .Dl # pfctl -k 0.0.0.0/0 -k host2
>  .Pp
> +.It Fl k Ar label | key | id
>  It is also possible to kill states by rule label, state key, or state ID.
>  In this mode the first
>  .Fl k
> 8<---8<---8<--8<
> 
> I don't insist on suggestion above, if you feel it is wrong/inconsistent
> go ahead and commit your patch as-is.
> 
> thanks and
> regards
> sasha
> 

hi.

it's a have your cake and eat it situation really. your proposal would
make the man page slightly more helpful and slightly less consistent.
i'm not keen to do it, though i am tempted.

on balance i would rather leave it as-is, and expect the reader
to read the description if they need reminding.

jmc

Re: pfctl(8): reduce -k

[Alexandr Nedvedicky]

Hello,

I'm fine with your pfctl.c change.

Although I like your brief version of manpage, wearing admin's hat, I'm
somewhat missing line:
>   -k host | network | label | key | id

so how about one small change below:

8<---8<---8<--8<
--- pfctl.8.new.jason   Sun Apr 23 12:10:00 2017
+++ pfctl.8.sashan  Sun Apr 23 12:15:56 2017
@@ -224,7 +224,7 @@
 .Fl K
 option may be specified, which will kill all the source tracking entries
 from the first host/network to the second.
-.It Fl k Ar key
+.It Fl k Ar host | network
 Kill all of the state entries originating from the
 host or network specified by
 .Ar key .
@@ -239,6 +239,7 @@
 .Pp
 .Dl # pfctl -k 0.0.0.0/0 -k host2
 .Pp
+.It Fl k Ar label | key | id
 It is also possible to kill states by rule label, state key, or state ID.
 In this mode the first
 .Fl k
8<---8<---8<--8<

I don't insist on suggestion above, if you feel it is wrong/inconsistent
go ahead and commit your patch as-is.

thanks and
regards
sasha

Re: delete pf states by exact flow info and speed it up

[Alexandr Nedvedicky]

Hello,

On Sun, Apr 23, 2017 at 12:18:07AM +0200, Hiltjo Posthuma wrote:
> On Sat, Apr 22, 2017 at 08:14:06AM +0900, YASUOKA Masahiko wrote:
> > On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
> > YASUOKA Masahiko  wrote:
> > > +int
> > > +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
> > > +{
> > (snip)
> > > + if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
> > > + hints.ai_family = AF_INET6;
> > > + *(sbs++) = *sbe = '\0';
> > 
> > The condition must be
> > 
> > if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
> > 
> > I will fix this before commit.
> > 
> > --yasuoka
> > 
> 
> Hey,
> 
> Shouldn't it be:
> 
>   if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(sbs, ']')) != 
> NULL) {
> ^
> 
> Since the ] should come after [ ?
> 

I think it does not matter. consider situation as follows:

s = "[ whatever ]"
sbs = strchr(s, '[');
/* sbs contains '[ whatever ]', s and sbs are same */

I would leave it on Yasuoka's personal choice...

thanks and
regards
sasha

Re: delete pf states by exact flow info and speed it up

[YASUOKA Masahiko]

On Sun, 23 Apr 2017 00:18:07 +0200
Hiltjo Posthuma  wrote:
> On Sat, Apr 22, 2017 at 08:14:06AM +0900, YASUOKA Masahiko wrote:
>> On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
>> YASUOKA Masahiko  wrote:
>> > +int
>> > +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
>> > +{
>> (snip)
>> > +  if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
>> > +  hints.ai_family = AF_INET6;
>> > +  *(sbs++) = *sbe = '\0';
>> 
>> The condition must be
>> 
>>  if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
>> 
>> I will fix this before commit.
> 
> Hey,
> 
> Shouldn't it be:
> 
>   if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(sbs, ']')) != 
> NULL) {
> ^
> 
> Since the ] should come after [ ?

Yes, it's better.  Thanks.

ok?

Index: sbin/pfctl/pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.340
diff -u -p -r1.340 pfctl.c
--- sbin/pfctl/pfctl.c  21 Apr 2017 23:22:49 -  1.340
+++ sbin/pfctl/pfctl.c  23 Apr 2017 09:02:20 -
@@ -762,7 +762,8 @@ pfctl_parse_host(char *str, struct pf_ru
hints.ai_socktype = SOCK_DGRAM; /* dummy */
hints.ai_flags = AI_NUMERICHOST;
 
-   if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
+   if ((sbs = strchr(s, '[')) != NULL &&
+   (sbe = strrchr(sbs, ']')) != NULL) {
hints.ai_family = AF_INET6;
*(sbs++) = *sbe = '\0';
} else if ((sbs = strchr(s, ':')) != NULL) {

Re: xinit(1): xdm(1) -> xenodm(1)

[Jason McIntyre]

On Fri, Apr 21, 2017 at 05:49:59PM -0500, Scott Cheloha wrote:
> Index: xinit.man
> ===
> RCS file: /cvs/xenocara/app/xinit/man/xinit.man,v
> retrieving revision 1.3
> diff -u -p -r1.3 xinit.man
> --- xinit.man   30 Aug 2015 13:32:02 -  1.3
> +++ xinit.man   21 Apr 2017 21:51:16 -
> @@ -44,7 +44,7 @@ xinit \- X Window System initializer
>  .SH DESCRIPTION
>  The \fBxinit\fP program is used to start the X Window System server and a 
> first
>  client program on systems that are not using a display manager such as
> -.BR xdm (__appmansuffix__)
> +.BR xenodm (__appmansuffix__)

i'll let the X people decide whether there's a case for change here or
not. but note that all this text is doing is giving an example of a
window manager and, as such, it's not wrong.

jmc