Re: sendsyslog file race

[Alexander Bluhm]

On Mon, Mar 27, 2017 at 05:39:27PM +0200, Mateusz Guzik wrote:
> The previous patch replaced multiple reads of the global var with just
> one read and had the result stored in a local variable, which then is
> read multiple times. Even though the compiler ended up emitting one read
> of the global, it perfectly legal to emit several, thus the bug can still
> reappear. This can be prevented as described above.

It is not a problem when the compiler creates multiple read
instructions for syslogf as long no function call is between them.
Nothing else can change the content of syslogf in parallel, we are
running with the big kernel lock.

It would be a problem if the compiler would do a read from the
global syslogf variable before and after a sleep.  But that would
be illegal in C.

When we will unlock this code for multi processor, we need lock,
memory barrier, per cpu memory, or whatever like you suggested.
But that does not happen now.

bluhm

Re: sendsyslog file race

[Mateusz Guzik]

On Sun, Mar 26, 2017 at 10:04 PM, Alexander Bluhm 
wrote:

> On Sun, Mar 26, 2017 at 05:00:12PM +0200, Mateusz Guzik wrote:
> > The patch is somewhat incorrect, although from what I checked it happens
> > to generate the expected outcome in terms of assembly (the global pointer
> > read only once and then a local copy accessed several times). You either
> > need a linux-like READ_ONCE macros which casts through a volatile pointer
> > or mark the global as volatile to prevent the compiler from issuing
> > multiple reads in the future.
>
> Note that our OpenBSD kernel is still implemented with a big kernel
> lock in most places.  So multi processor thinking and locking is
> not necessary.  The execution order can only be interrupted by
> hardware interrups or explicitly rescheduling with tsleep(9).  Here
> the sleep is hidden in copyin(), mallocarray(), sosend(), malloc(),
> ktrgenio().
>
> Interrupts are not relevant, they never change syslogf.  As tsleep()
> is a function call, it automatically acts as compiler barrier.  So
> volatile is not needed.
>
>
The previous patch replaced multiple reads of the global var with just
one read and had the result stored in a local variable, which then is
read multiple times. Even though the compiler ended up emitting one read
of the global, it perfectly legal to emit several, thus the bug can still
reappear. This can be prevented as described above.


> > Remaining ses sof syslogf are also super fishy:
> > 1. logclose
> > 2. logioctl -> LIOCSFD
> > FRELE(syslogf, p);
>
> A few months ago I would have said FRELE() does not sleep.  I think
> this is currently still the case as we do not grab the netlock for
> socketpairs.  But we did for a while.
>
> As we are heading towards multi processor in kernel, I would suggest
> this diff.  It orders FREF() and FRELE() in a way that we only
> operate on refcounted global variables.  Although not necessary
> with the current sleeping points, I think it results in more robust
> code.


With aforementioned caveat ignored, the patch indeed fixes the problem.

I skimmed through and found few *sleep calls, but perhaps they cannot
end up being executed for the type of socket accepted here.

-- 
Mateusz Guzik 

Re: sendsyslog file race

[Theo de Raadt]

> On Mon, Mar 27, 2017 at 05:39:27PM +0200, Mateusz Guzik wrote:
> > The previous patch replaced multiple reads of the global var with just
> > one read and had the result stored in a local variable, which then is
> > read multiple times. Even though the compiler ended up emitting one read
> > of the global, it perfectly legal to emit several, thus the bug can still
> > reappear. This can be prevented as described above.
> 
> It is not a problem when the compiler creates multiple read
> instructions for syslogf as long no function call is between them.
> Nothing else can change the content of syslogf in parallel, we are
> running with the big kernel lock.
> 
> It would be a problem if the compiler would do a read from the
> global syslogf variable before and after a sleep.  But that would
> be illegal in C.
> 
> When we will unlock this code for multi processor, we need lock,
> memory barrier, per cpu memory, or whatever like you suggested.
> But that does not happen now.

This really has nothing to do with the biglock.

As the biglock gets pushed out of this layer, other locks will be
added to ensure that the VFS functions can be safely call, those will
satisfy the conditions.  There's no point creating needless
contortions.

arm64 kernel W^X, for real this time?

[Mark Kettenis]

So my previous diff only did W^X for the bootstrap page tables.  Since
these page tables get replaced quickly afterwards, W^X wasn't really
enforced.  The diff below hopefully does a better job.  It removes
some commented out code that was clearly trying to do the same thing
as pmap_map_stolen().  It also kills a bunch of debug printfs that I
deem unnecessary at this point and weren't showing up on the console
anyway.

ok?


Index: arch/arm64/arm64/pmap.c
===
RCS file: /cvs/src/sys/arch/arm64/arm64/pmap.c,v
retrieving revision 1.28
diff -u -p -r1.28 pmap.c
--- arch/arm64/arm64/pmap.c 24 Mar 2017 19:48:01 -  1.28
+++ arch/arm64/arm64/pmap.c 27 Mar 2017 16:51:39 -
@@ -1132,6 +1132,9 @@ CTASSERT(sizeof(struct pmapvp0) == 8192)
 int mappings_allocated = 0;
 int pted_allocated = 0;
 
+extern char __text_start[], _etext[];
+extern char __rodata_start[], _erodata[];
+
 vaddr_t
 pmap_bootstrap(long kvo, paddr_t lpt1,  long kernelstart, long kernelend,
 long ram_start, long ram_end)
@@ -1241,8 +1244,6 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
// enable mappings for existing 'allocated' mapping in the bootstrap
// page tables
extern uint64_t *pagetable;
-   extern char __text_start[], _etext[];
-   extern char __rodata_start[], _erodata[];
extern char _end[];
vp2 = (void *)((long) + kvo);
struct mem_region *mp;
@@ -1273,36 +1274,15 @@ pmap_bootstrap(long kvo, paddr_t lpt1,  
}
}
 
-   // At this point we are still running on the bootstrap page tables
-   // however all memory allocated for the final page tables is
-   // 'allocated' and should now be mapped
-   // This means we are able to use the virtual addressing to
-   // enter the final mappings into the new mapping tables.
-
-#if 0
-   vm_prot_t prot;
-
-   for (mp = pmap_allocated; mp->size != 0; mp++) {
-   vaddr_t va;
-   paddr_t pa;
-   vsize_t size;
-   extern char *etext();
-   //printf("mapping %08x sz %08x\n", mp->start, mp->size);
-   for (pa = mp->start, va = pa + kvo, size = mp->size & ~0xfff;
-   size > 0;
-   va += PAGE_SIZE, pa+= PAGE_SIZE, size -= PAGE_SIZE)
-   {
-   pa = va - kvo;
-   prot = PROT_READ|PROT_WRITE;
-   if ((vaddr_t)va >= (vaddr_t)kernelstart &&
-   (vaddr_t)va < (vaddr_t)etext)
-   prot |= PROT_EXEC; // XXX remove WRITE?
-   pmap_kenter_cache(va, pa, prot, PMAP_CACHE_WB);
-   }
-   }
-#endif
pmap_avail_fixup();
 
+   /*
+* At this point we are still running on the bootstrap page
+* tables however all memory for the final page tables is
+* 'allocated' and should now be mapped.  This means we are
+* able to use the virtual addressing to enter the final
+* mappings into the new mapping tables.
+*/
vstart = pmap_map_stolen(kernelstart);
 
void (switch_mmu_kernel)(long);
@@ -2117,42 +2097,33 @@ pmap_steal_avail(size_t size, int align,
 vaddr_t
 pmap_map_stolen(vaddr_t kernel_start)
 {
-   int prot;
struct mem_region *mp;
-   uint64_t pa, va, e;
-   extern char *etext();
-
+   paddr_t pa;
+   vaddr_t va;
+   uint64_t e;
 
-   int oldprot = 0;
-   printf("mapping self\n");
for (mp = pmap_allocated; mp->size; mp++) {
-   printf("start %16llx end %16llx\n", mp->start, mp->start + 
mp->size);
-   printf("exe range %16llx %16llx\n", kernel_start,
-   (uint64_t));
for (e = 0; e < mp->size; e += PAGE_SIZE) {
-   /* XXX - is this a kernel text mapping? */
-   /* XXX - Do we care about KDB ? */
+   int prot = PROT_READ | PROT_WRITE;
+
pa = mp->start + e;
va = pa - pmap_avail_kvo;
+
if (va < VM_MIN_KERNEL_ADDRESS ||
va >= VM_MAX_KERNEL_ADDRESS)
continue;
-   if ((vaddr_t)va >= (vaddr_t)kernel_start &&
-   (vaddr_t)va < (vaddr_t)) {
-   prot = PROT_READ|PROT_WRITE|
-   PROT_EXEC;
-   } else {
-   prot = PROT_READ|PROT_WRITE;
-   }
-   if (prot != oldprot) {
-   printf("mapping  v %16llx p %16llx prot %x\n", 
va,
-   pa, prot);
-   oldprot = prot;
-   }
+
+   if (va >= (vaddr_t)__text_start &&
+

Re: sendsyslog file race

[Mateusz Guzik]

On Mon, Mar 27, 2017 at 6:09 PM, Alexander Bluhm 
wrote:

> On Mon, Mar 27, 2017 at 05:39:27PM +0200, Mateusz Guzik wrote:
> > The previous patch replaced multiple reads of the global var with just
> > one read and had the result stored in a local variable, which then is
> > read multiple times. Even though the compiler ended up emitting one read
> > of the global, it perfectly legal to emit several, thus the bug can still
> > reappear. This can be prevented as described above.
>
> It is not a problem when the compiler creates multiple read
> instructions for syslogf as long no function call is between them.
> Nothing else can change the content of syslogf in parallel, we are
> running with the big kernel lock.
>
> It would be a problem if the compiler would do a read from the
> global syslogf variable before and after a sleep.  But that would
> be illegal in C.


Agreed. I somehow brainfarted myself into not realising it really can't
reload the var across a func call which would be needed to give cpu to
someone else.

Sorry for the noise in that bit.

-- 
Mateusz Guzik 

Re: pfctl: Fix function name in errx(3) message

[Sebastian Benoit]

Klemens Nanni(k...@posteo.org) on 2017.03.25 12:39:48 +0100:
> Index: pfctl.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
> retrieving revision 1.338
> diff -u -p -r1.338 pfctl.c
> --- pfctl.c 26 Jan 2017 08:24:34 -  1.338
> +++ pfctl.c 25 Mar 2017 11:37:01 -
> @@ -753,7 +753,7 @@ pfctl_show_rules(int dev, char *path, in
>memset(, 0, sizeof(pr));
>if (anchorname[0] == '/') {
>if ((npath = calloc(1, PATH_MAX)) == NULL)
> -   errx(1, "pfctl_rules: calloc");
> +   errx(1, "pfctl_show_rules: calloc");
>strlcpy(npath, anchorname, PATH_MAX);
>} else {
>if (path[0])
> 

i commited a err(1, "calloc") instead.
thanks!

Re: pfctl: Fix function name in errx(3) message

[Gleydson Soares]

> What about the other calls to calloc(3); shouldn't we keep their
> respective error messages consistent?

seems that several err()/errx() calls in pfctl code are hard-coding
the function name...

instead of hard code, maybe those calls should be changed to:
errx(1, "%s: anystring", __func__)

Re: pfctl: Fix function name in errx(3) message

[Ingo Schwarze]

Hi,

Gleydson Soares wrote on Mon, Mar 27, 2017 at 04:43:28PM -0300:

> instead of hard code, maybe those calls should be changed to:
> errx(1, "%s: anystring", __func__)

the usual OpenBSD idiom is

if ((p = malloc(size)) == NULL)
err(1, NULL);

see EXAMPLES in malloc(3), simply because the information about
whether it was malloc or calloc or whatever that failed is useless,
and the name of the function where it failed is almost never useful
either.  It's just a matter of chance when exactly memory is
exhausted.  So KISS is the usual approach in this respect.

But it's certainly not a big deal either way.

Yours,
  Ingo

Re: pfctl: Fix function name in errx(3) message

[Klemens Nanni]

On Mon, Mar 27, 2017 at 07:39:09PM +0200, Sebastian Benoit wrote:

i commited a err(1, "calloc") instead.

What about the other calls to calloc(3); shouldn't we keep their
respective error messages consistent?

Re: Fix broken example link in packages.7

[Frederic Cambus]

On Wed, Mar 01, 2017 at 11:16:57PM +, Stuart Henderson wrote:

> > -# export PKG_PATH=ftp://ftp.openbsd.org/pub/OpenBSD/5.2/packages/i386/
> > +# export PKG_PATH=http://ftp.openbsd.org/pub/OpenBSD/%c/packages/%a/
> 
> Hmm - if you're going to trust autodetection of snapshots vs release
> anyway (as you do with %c), you could simply "export 
> PKG_PATH=ftp.openbsd.org" ...
> 
> Not sure which is better though.

Sounds good to me, espie@ likes it as well, and this also addresses the
concern pointed by Raf Czlonka. So here is a new diff.

Comments? OK?

Index: share/man/man7/packages.7
===
RCS file: /cvs/src/share/man/man7/packages.7,v
retrieving revision 1.40
diff -u -p -r1.40 packages.7
--- share/man/man7/packages.7   24 Oct 2015 08:44:49 -  1.40
+++ share/man/man7/packages.7   27 Mar 2017 19:14:12 -
@@ -240,7 +240,7 @@ are supported: pointing
 .Ev PKG_PATH
 to a distant package repository, e.g.,
 .Bd -literal -offset 1n
-# export PKG_PATH=ftp://ftp.openbsd.org/pub/OpenBSD/5.2/packages/i386/
+# export PKG_PATH=ftp.openbsd.org
 .Ed
 .Pp
 will let

[PATCH] Fix audio on MacBook Pro 5,5

[Manav Rathi]

The following changes to the azalia(4) driver were required
to hear audio on a MacbookPro 5,5 laptop:


--- azalia_codec.c.orig Wed Dec  2 08:41:32 2015
+++ azalia_codec.c  Thu Mar 16 17:58:38 2017
@@ -66,7 +66,8 @@
case 0x10134206:
this->name = "Cirrus Logic CS4206";
if (this->subid == 0xcb8910de ||/* APPLE_MBA3_1 */
-   this->subid == 0x72708086) {/* APPLE_MBA4_1 */
+   this->subid == 0x72708086 ||/* APPLE_MBA4_1 */
+   this->subid == 0xcb7910de) {/* APPLE_MBP5_5 */
this->qrks |= AZ_QRK_GPIO_UNMUTE_1 |
AZ_QRK_GPIO_UNMUTE_3;
}

Re: regarding OpenSSL License change

[Marc Espie]

On Sun, Mar 26, 2017 at 08:49:39PM -0500, Jimmy Hess wrote:
> > > From: "Constantine A. Murenin" 
> > > If we do not hear from you, we will assume that you have no objection.
> > Is this for real?!
> > Who do they think they are?  ...
> >People should not bother to respond to such nonsense, and then sue
> > OpenSSL for obvious copyright infringement
> 
> I think "Don't bother to respond, and plan to sue" would be a poor
> response,  that would just hurt everyone involved.Of course
> silence does not generally grant permission. But the people in
> that project might be able to convincingly deliver some kind of
> argument that they've had implicit or "understood"  permissions made
> at time of submission to use contributions however the project
> collectively agrees to use them.

Bullshit.

The FSF, who understands this kind of stuff, was very careful about that 
with their paperwork.

All other organisations that ever wanted to change licences or audit their
trees had to spend quite a lot of time contacting authors and fixing things.

Basically, the only thing you can do when an author doesn't agree is rewrite
stuff from scratch.

The OpenSSL authors don't have a magic wand that allows them to do whatever
they please.  For that matter, if they DID have a magic wand, a much better
use of it would be to zap away all the bugs in their code.

> Also, there is no work-around for a contributor denying.   They might
> have the  idea of simply Removing and Replacing a  contribution  (Even
> if you can accurately identify and rewrite specific lines of code from
> a certain author)  does not  necessarily make the distribution
> Non-infringing,   As  later code is likely to have built on top of
> earlier code.

Rewrite from scratch.   The importance of code lines is generally greatly
exaggerated.  Non regression tests are generally way more precious than
actual code.

[PATCH] pcidump - read and write registers

[Simon Mages]

Hi,

right now i use the following diff to poke around in the PCIe config
space. This diff enables
pcidump to read and write to a register. So far i used this mainly to
play with the Advanced
Error Reporting Capability some devices have.

$ pcidump 4:0:0:104
 4:0:0: Broadcom BCM5754
0x0104: 0x0010
This bit indicates an "Unsupported Request Error", the register
queried here is the
"Uncorrectable Error Status Register".

# pcidump 4:0:0:104:0x0010
 4:0:0: Broadcom BCM5754
0x0104: 0x
pcidump shows the new value of the register after writing. By writing
a 1 to a status bit it
gets reset.

I implemented a check for the current securelevel because writing to
/dev/pci is only possible
for a securelevel smaller than 1.

I think this functionality can come in handy for people
writing/modifying device drivers.

Index: pcidump.8
===
--- pcidump.8   16 Jul 2013 11:13:34 -  1.12
+++ pcidump.8   27 Mar 2017 11:27:35 -
@@ -26,7 +26,7 @@
 .Op Fl x | xx | xxx
 .Op Fl d Ar pcidev
 .Sm off
-.Op Ar bus : dev : func
+.Op Ar bus : dev : func [ : reg [ : val ] ]
 .Sm on
 .Nm pcidump
 .Fl r Ar file
@@ -69,16 +69,29 @@ Shows a hexadecimal dump of the full PCI
 Shows a hexadecimal dump of the PCIe extended config space.
 .It Xo
 .Sm off
-.Ar bus : dev : func
+.Ar bus : dev : func [ : reg [ : val ] ]
 .Sm on
 .Xc
 Show information about the PCI device specified by the tuple given on
-the command line.
+the command line. If
+.Pa reg
+is used, the value of this register in the configuration space of
+.Pa func
+gets printed. If
+.Pa val
+is used, the register specified by
+.Pa reg
+will be loaded with the value specified by
+.Pa val .
 If the
 .Fl d
 option is not given,
 .Pa /dev/pci
 is used.
+.It Xo
+.Xc
+The configuration space can only be written in a securelevel(7) lower
+than 1.
 .El
 .Sh FILES
 .Bl -tag -width /dev/pci* -compact
@@ -86,7 +99,8 @@ is used.
 Device files for accessing PCI domains.
 .El
 .Sh SEE ALSO
-.Xr pci 4
+.Xr pci 4 ,
+.Xr securelevel 7
 .Sh HISTORY
 The
 .Nm
Index: pcidump.c
===
--- pcidump.c   25 Mar 2017 07:33:46 -  1.43
+++ pcidump.c   27 Mar 2017 11:24:10 -
@@ -19,6 +19,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 

 #include  /* need NULL for  */

@@ -37,19 +39,27 @@

 #define PCIDEV "/dev/pci"

+#define PCI_CONFIG_SPACE_BEGIN 0x0
+#define PCIE_CONFIG_SPACE_END  (PCIE_CONFIG_SPACE_SIZE - 1)
+#define PCI_CONFIG_ALIGNMENT   0x4
+#define REG_ALIGNMENT_OK(x)((x) % PCI_CONFIG_ALIGNMENT ? 0 : 1)
+
 #ifndef nitems
 #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0]))
 #endif

 __dead void usage(void);
+int get_securelevel(void);
 void scanpcidomain(void);
-int probe(int, int, int);
+int probe(int, int, int, int, int);
+void chreg(int, int, int, int, int);
 void dump(int, int, int);
 void hexdump(int, int, int, int);
-const char *str2busdevfunc(const char *, int *, int *, int *);
+const char *str2busdevfunc(const char *, int *, int *, int *, int *, int *);
 int pci_nfuncs(int, int);
 int pci_read(int, int, int, u_int32_t, u_int32_t *);
 int pci_readmask(int, int, int, u_int32_t, u_int32_t *);
+int pci_write(int, int, int, u_int32_t, u_int32_t);
 void dump_caplist(int, int, int, u_int8_t);
 void dump_pci_powerstate(int, int, int, uint8_t);
 void dump_pcie_linkspeed(int, int, int, uint8_t);
@@ -67,7 +77,8 @@ usage(void)
extern char *__progname;

fprintf(stderr,
-   "usage: %s [-v] [-x | -xx | -xxx] [-d pcidev] [bus:dev:func]\n"
+   "usage: %s [-v] [-x | -xx | -xxx] [-d pcidev]"
+   " [bus:dev:func[:reg[:val]]]\n"
"   %s -r file [-d pcidev] bus:dev:func\n",
__progname, __progname);
exit(1);
@@ -139,7 +150,7 @@ int
 main(int argc, char *argv[])
 {
int nfuncs;
-   int bus, dev, func;
+   int bus, dev, func, reg = -1, val = -1;
char pcidev[PATH_MAX] = PCIDEV;
char *romfile = NULL;
const char *errstr;
@@ -186,7 +197,10 @@ main(int argc, char *argv[])
dumpall = 0;

if (dumpall == 0) {
-   pcifd = open(pcidev, O_RDONLY, 0777);
+   if (get_securelevel() < 1)
+   pcifd = open(pcidev, O_RDWR, 0777);
+   else
+   pcifd = open(pcidev, O_RDONLY, 0777);
if (pcifd == -1)
err(1, "%s", pcidev);
} else {
@@ -207,7 +221,7 @@ main(int argc, char *argv[])
}

if (argc == 1) {
-   errstr = str2busdevfunc(argv[0], , , );
+   errstr = str2busdevfunc(argv[0], , , , , );
if (errstr != NULL)
errx(1, "\"%s\": %s", argv[0], errstr);

@@ -217,7 +231,7 @@ main(int argc, char *argv[])
else if (romfile)
error = dump_rom(bus, dev, func);
else
- 

Re: [PATCH] pcidump - read and write registers

[Simon Mages]

Ah ok, good to know.

Thanks anyway.


2017-03-27 14:57 GMT+02:00, Mark Kettenis :
>> From: Simon Mages 
>> Date: Mon, 27 Mar 2017 13:57:54 +0200
>>
>> Hi,
>>
>> right now i use the following diff to poke around in the PCIe config
>> space. This diff enables
>> pcidump to read and write to a register. So far i used this mainly to
>> play with the Advanced
>> Error Reporting Capability some devices have.
>>
>> $ pcidump 4:0:0:104
>>  4:0:0: Broadcom BCM5754
>> 0x0104: 0x0010
>> This bit indicates an "Unsupported Request Error", the register
>> queried here is the
>> "Uncorrectable Error Status Register".
>>
>> # pcidump 4:0:0:104:0x0010
>>  4:0:0: Broadcom BCM5754
>> 0x0104: 0x
>> pcidump shows the new value of the register after writing. By writing
>> a 1 to a status bit it
>> gets reset.
>>
>> I implemented a check for the current securelevel because writing to
>> /dev/pci is only possible
>> for a securelevel smaller than 1.
>>
>> I think this functionality can come in handy for people
>> writing/modifying device drivers.
>
> Sorry, but no.  This is not going to happen.  The kernel interface to
> write to pci config space is only there to support X.  And even that
> support is likely to disappear at some point.
>
>> Index: pcidump.8
>> ===
>> --- pcidump.816 Jul 2013 11:13:34 -  1.12
>> +++ pcidump.827 Mar 2017 11:27:35 -
>> @@ -26,7 +26,7 @@
>>  .Op Fl x | xx | xxx
>>  .Op Fl d Ar pcidev
>>  .Sm off
>> -.Op Ar bus : dev : func
>> +.Op Ar bus : dev : func [ : reg [ : val ] ]
>>  .Sm on
>>  .Nm pcidump
>>  .Fl r Ar file
>> @@ -69,16 +69,29 @@ Shows a hexadecimal dump of the full PCI
>>  Shows a hexadecimal dump of the PCIe extended config space.
>>  .It Xo
>>  .Sm off
>> -.Ar bus : dev : func
>> +.Ar bus : dev : func [ : reg [ : val ] ]
>>  .Sm on
>>  .Xc
>>  Show information about the PCI device specified by the tuple given on
>> -the command line.
>> +the command line. If
>> +.Pa reg
>> +is used, the value of this register in the configuration space of
>> +.Pa func
>> +gets printed. If
>> +.Pa val
>> +is used, the register specified by
>> +.Pa reg
>> +will be loaded with the value specified by
>> +.Pa val .
>>  If the
>>  .Fl d
>>  option is not given,
>>  .Pa /dev/pci
>>  is used.
>> +.It Xo
>> +.Xc
>> +The configuration space can only be written in a securelevel(7) lower
>> +than 1.
>>  .El
>>  .Sh FILES
>>  .Bl -tag -width /dev/pci* -compact
>> @@ -86,7 +99,8 @@ is used.
>>  Device files for accessing PCI domains.
>>  .El
>>  .Sh SEE ALSO
>> -.Xr pci 4
>> +.Xr pci 4 ,
>> +.Xr securelevel 7
>>  .Sh HISTORY
>>  The
>>  .Nm
>> Index: pcidump.c
>> ===
>> --- pcidump.c25 Mar 2017 07:33:46 -  1.43
>> +++ pcidump.c27 Mar 2017 11:24:10 -
>> @@ -19,6 +19,8 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>>
>>  #include   /* need NULL for  */
>>
>> @@ -37,19 +39,27 @@
>>
>>  #define PCIDEV  "/dev/pci"
>>
>> +#define PCI_CONFIG_SPACE_BEGIN  0x0
>> +#define PCIE_CONFIG_SPACE_END   (PCIE_CONFIG_SPACE_SIZE - 1)
>> +#define PCI_CONFIG_ALIGNMENT0x4
>> +#define REG_ALIGNMENT_OK(x) ((x) % PCI_CONFIG_ALIGNMENT ? 0 : 1)
>> +
>>  #ifndef nitems
>>  #define nitems(_a)  (sizeof((_a)) / sizeof((_a)[0]))
>>  #endif
>>
>>  __dead void usage(void);
>> +int get_securelevel(void);
>>  void scanpcidomain(void);
>> -int probe(int, int, int);
>> +int probe(int, int, int, int, int);
>> +void chreg(int, int, int, int, int);
>>  void dump(int, int, int);
>>  void hexdump(int, int, int, int);
>> -const char *str2busdevfunc(const char *, int *, int *, int *);
>> +const char *str2busdevfunc(const char *, int *, int *, int *, int *, int
>> *);
>>  int pci_nfuncs(int, int);
>>  int pci_read(int, int, int, u_int32_t, u_int32_t *);
>>  int pci_readmask(int, int, int, u_int32_t, u_int32_t *);
>> +int pci_write(int, int, int, u_int32_t, u_int32_t);
>>  void dump_caplist(int, int, int, u_int8_t);
>>  void dump_pci_powerstate(int, int, int, uint8_t);
>>  void dump_pcie_linkspeed(int, int, int, uint8_t);
>> @@ -67,7 +77,8 @@ usage(void)
>>  extern char *__progname;
>>
>>  fprintf(stderr,
>> -"usage: %s [-v] [-x | -xx | -xxx] [-d pcidev] [bus:dev:func]\n"
>> +"usage: %s [-v] [-x | -xx | -xxx] [-d pcidev]"
>> +" [bus:dev:func[:reg[:val]]]\n"
>>  "   %s -r file [-d pcidev] bus:dev:func\n",
>>  __progname, __progname);
>>  exit(1);
>> @@ -139,7 +150,7 @@ int
>>  main(int argc, char *argv[])
>>  {
>>  int nfuncs;
>> -int bus, dev, func;
>> +int bus, dev, func, reg = -1, val = -1;
>>  char pcidev[PATH_MAX] = PCIDEV;
>>  char *romfile = NULL;
>>  const char *errstr;
>> @@ -186,7 +197,10 @@ main(int argc, char *argv[])
>>  dumpall = 0;
>>
>>  if 

Re: regarding OpenSSL License change

[Marc Espie]

On Thu, Mar 23, 2017 at 10:18:26AM -0600, Theo de Raadt wrote:
> Lots of people have been receiving emails like the one below.
[...]
> Date: Wed, 22 Mar 2017 16:48:10 -0400
> From: lice...@openssl.org
> To: dera...@cvs.openbsd.org
> Subject: OpenSSL License change
> Message-ID: <20170322204810.ra49wtmwn%lice...@openssl.org>
> User-Agent: s-nail v14.8.6
> Status: O
> 
> Hello!
> 
> This mail is coming from the OpenSSL development team.
> 
> This is a pre-release email before we "go public."  In particular,
> the most recent blog entry, listed below, is not yet available.  But we
> thought, as an important downstream fork, that we'd give you the courtesy
> of participating early.
> 
> We are working to change the license for OpenSSL. We want to move from
> the current license (which is custom-written and has some uncommon
> requirements on end-users), to the widely-accepted and common 
> Apache License (version 2).  You can find some explanation in
> our blog entries:

Thinking some more about it, the step from "custom written licence, weird
wording" into "Apache 2, wide-spread and acceptable" is very hypocritical.

If I understand things correctly, it's mainly a specific choice from one
guy, and I believe it should be scrutinized more: why choose the
Apache License v2, which is very controversial, instead of a more widely
accepted license, such as the 2 clause BSD / ISC license ?   


I would very much like to know if this is a misguided clueless
attempt to simplify things (we're talking about openssl, so this
wouldn't be too far-fetched), or whether there's an actual further
agenda pushed by some organisation with deep pockets which is ready
to "sponsor" some openssl developers if they manage to get things
moving in the right direction.


-- 
Marc

Re: [PATCH] pcidump - read and write registers

[Mark Kettenis]

> From: Simon Mages 
> Date: Mon, 27 Mar 2017 13:57:54 +0200
> 
> Hi,
> 
> right now i use the following diff to poke around in the PCIe config
> space. This diff enables
> pcidump to read and write to a register. So far i used this mainly to
> play with the Advanced
> Error Reporting Capability some devices have.
> 
> $ pcidump 4:0:0:104
>  4:0:0: Broadcom BCM5754
> 0x0104: 0x0010
> This bit indicates an "Unsupported Request Error", the register
> queried here is the
> "Uncorrectable Error Status Register".
> 
> # pcidump 4:0:0:104:0x0010
>  4:0:0: Broadcom BCM5754
> 0x0104: 0x
> pcidump shows the new value of the register after writing. By writing
> a 1 to a status bit it
> gets reset.
> 
> I implemented a check for the current securelevel because writing to
> /dev/pci is only possible
> for a securelevel smaller than 1.
> 
> I think this functionality can come in handy for people
> writing/modifying device drivers.

Sorry, but no.  This is not going to happen.  The kernel interface to
write to pci config space is only there to support X.  And even that
support is likely to disappear at some point.

> Index: pcidump.8
> ===
> --- pcidump.8 16 Jul 2013 11:13:34 -  1.12
> +++ pcidump.8 27 Mar 2017 11:27:35 -
> @@ -26,7 +26,7 @@
>  .Op Fl x | xx | xxx
>  .Op Fl d Ar pcidev
>  .Sm off
> -.Op Ar bus : dev : func
> +.Op Ar bus : dev : func [ : reg [ : val ] ]
>  .Sm on
>  .Nm pcidump
>  .Fl r Ar file
> @@ -69,16 +69,29 @@ Shows a hexadecimal dump of the full PCI
>  Shows a hexadecimal dump of the PCIe extended config space.
>  .It Xo
>  .Sm off
> -.Ar bus : dev : func
> +.Ar bus : dev : func [ : reg [ : val ] ]
>  .Sm on
>  .Xc
>  Show information about the PCI device specified by the tuple given on
> -the command line.
> +the command line. If
> +.Pa reg
> +is used, the value of this register in the configuration space of
> +.Pa func
> +gets printed. If
> +.Pa val
> +is used, the register specified by
> +.Pa reg
> +will be loaded with the value specified by
> +.Pa val .
>  If the
>  .Fl d
>  option is not given,
>  .Pa /dev/pci
>  is used.
> +.It Xo
> +.Xc
> +The configuration space can only be written in a securelevel(7) lower
> +than 1.
>  .El
>  .Sh FILES
>  .Bl -tag -width /dev/pci* -compact
> @@ -86,7 +99,8 @@ is used.
>  Device files for accessing PCI domains.
>  .El
>  .Sh SEE ALSO
> -.Xr pci 4
> +.Xr pci 4 ,
> +.Xr securelevel 7
>  .Sh HISTORY
>  The
>  .Nm
> Index: pcidump.c
> ===
> --- pcidump.c 25 Mar 2017 07:33:46 -  1.43
> +++ pcidump.c 27 Mar 2017 11:24:10 -
> @@ -19,6 +19,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> 
>  #include/* need NULL for  */
> 
> @@ -37,19 +39,27 @@
> 
>  #define PCIDEV   "/dev/pci"
> 
> +#define PCI_CONFIG_SPACE_BEGIN   0x0
> +#define PCIE_CONFIG_SPACE_END(PCIE_CONFIG_SPACE_SIZE - 1)
> +#define PCI_CONFIG_ALIGNMENT 0x4
> +#define REG_ALIGNMENT_OK(x)  ((x) % PCI_CONFIG_ALIGNMENT ? 0 : 1)
> +
>  #ifndef nitems
>  #define nitems(_a)   (sizeof((_a)) / sizeof((_a)[0]))
>  #endif
> 
>  __dead void usage(void);
> +int get_securelevel(void);
>  void scanpcidomain(void);
> -int probe(int, int, int);
> +int probe(int, int, int, int, int);
> +void chreg(int, int, int, int, int);
>  void dump(int, int, int);
>  void hexdump(int, int, int, int);
> -const char *str2busdevfunc(const char *, int *, int *, int *);
> +const char *str2busdevfunc(const char *, int *, int *, int *, int *, int *);
>  int pci_nfuncs(int, int);
>  int pci_read(int, int, int, u_int32_t, u_int32_t *);
>  int pci_readmask(int, int, int, u_int32_t, u_int32_t *);
> +int pci_write(int, int, int, u_int32_t, u_int32_t);
>  void dump_caplist(int, int, int, u_int8_t);
>  void dump_pci_powerstate(int, int, int, uint8_t);
>  void dump_pcie_linkspeed(int, int, int, uint8_t);
> @@ -67,7 +77,8 @@ usage(void)
>   extern char *__progname;
> 
>   fprintf(stderr,
> - "usage: %s [-v] [-x | -xx | -xxx] [-d pcidev] [bus:dev:func]\n"
> + "usage: %s [-v] [-x | -xx | -xxx] [-d pcidev]"
> + " [bus:dev:func[:reg[:val]]]\n"
>   "   %s -r file [-d pcidev] bus:dev:func\n",
>   __progname, __progname);
>   exit(1);
> @@ -139,7 +150,7 @@ int
>  main(int argc, char *argv[])
>  {
>   int nfuncs;
> - int bus, dev, func;
> + int bus, dev, func, reg = -1, val = -1;
>   char pcidev[PATH_MAX] = PCIDEV;
>   char *romfile = NULL;
>   const char *errstr;
> @@ -186,7 +197,10 @@ main(int argc, char *argv[])
>   dumpall = 0;
> 
>   if (dumpall == 0) {
> - pcifd = open(pcidev, O_RDONLY, 0777);
> + if (get_securelevel() < 1)
> + pcifd = open(pcidev, O_RDWR, 0777);
> + else
> + pcifd = open(pcidev, O_RDONLY, 0777);
>   if 

Re: pf: percpu anchor stacks

[Mike Belopuhov]

On Fri, Mar 24, 2017 at 12:19 +0100, Alexandr Nedvedicky wrote:
> Hello,
> 
> I'm attaching patch, which removes stack-as-a-global variable.
> it's updated patch [1] to current tree.
> 
> sorry for being pushy advocating my old, rusty patch.
>

I think your diff is the way to go indeed.  If we can avoid
using the global stack altogether, then all the better.
This diff also splits giant pf_test_rule into several chunks
which is a good thing in my opinion.

A few random comments:

 - some lines appear to be longer than 80 symbols;

 - "cx" is an uncommon abbreviation for a "context" in OpenBSD,
   we normally use "ctx";

 - PF_TEST_ATTRIB could use a "continue" statement instead of
   the goto;

 - instead of checking "rv" against 0 in the "break on quick
   rule or failure" I'd like to see an actual check against
   PF_TEST_* values so that it's grep'able;

 - s/test_status/action/ as it's done everywhere else?

 - I'm not certain I like extra set of PASS/BLOCK macros.
   I know you want to represent the "quick" pass separately,
   but perhaps there's a way to do it while using PF_PASS...

Does this pass pfctl regress tests?

While I haven't noticed anything criminal here, it makes me
wonder if it'd be possible to do this change in a few steps:
factor out rule maching from pf_test_rule and then bring in
anchor changes?

> thanks and
> regards
> sasha
> 
> [1] 
> http://openbsd-archive.7691.n7.nabble.com/Re-PF-SMP-making-anchor-stack-multithreaded-tt275915.html#none
> 
> 8<---8<---8<--8<
> diff -r d6e3a6338889 src/sys/net/pf.c
> --- a/src/sys/net/pf.cMon Mar 20 01:10:40 2017 +0100
> +++ b/src/sys/net/pf.cFri Mar 24 11:28:18 2017 +0100
> @@ -119,12 +119,37 @@ u_char   pf_tcp_secret[16];
>  int   pf_tcp_secret_init;
>  int   pf_tcp_iss_off;
>  
> -struct pf_anchor_stackframe {
> - struct pf_ruleset   *rs;
> - struct pf_rule  *r;
> - struct pf_anchor_node   *parent;
> - struct pf_anchor*child;
> -} pf_anchor_stack[64];
> +struct pf_test_ctx {
> + int test_status;
> + struct pf_pdesc *pd;
> + struct pf_rule_actions  act;
> + u_int8_ticmpcode;
> + u_int8_ticmptype;
> + int icmp_dir;
> + int state_icmp;
> + int tag;
> + u_short reason;
> + struct pf_rule_item *ri;
> + struct pf_src_node  *sns[PF_SN_MAX];
> + struct pf_rule_slistrules;
> + struct pf_rule  *nr;
> + struct pf_rule  **rm;
> + struct pf_rule  *a;
> + struct pf_rule  **am;
> + struct pf_ruleset   **rsm;
> + struct pf_ruleset   *arsm;
> + struct pf_ruleset   *aruleset;
> + struct tcphdr   *th;
> + int  depth;
> +};
> +
> +#define  PF_ANCHOR_STACK_MAX 64
> +
> +enum {
> + PF_TEST_FAIL = -1,
> + PF_TEST_OK,
> + PF_TEST_QUICK
> +};
>  
>  struct pool   pf_src_tree_pl, pf_rule_pl, pf_queue_pl;
>  struct pool   pf_state_pl, pf_state_key_pl, pf_state_item_pl;
> @@ -211,11 +236,8 @@ struct pf_state  *pf_find_state(struct p
>   struct pf_state_key_cmp *, u_int, struct mbuf *);
>  int   pf_src_connlimit(struct pf_state **);
>  int   pf_match_rcvif(struct mbuf *, struct pf_rule *);
> -void  pf_step_into_anchor(int *, struct pf_ruleset **,
> - struct pf_rule **, struct pf_rule **);
> -int   pf_step_out_of_anchor(int *, struct pf_ruleset **,
> -  struct pf_rule **, struct pf_rule **,
> -  int *);
> +int   pf_step_into_anchor(struct pf_test_ctx *, struct 
> pf_rule *);
> +int   pf_match_rule(struct pf_test_ctx *, struct pf_ruleset 
> *);
>  void  pf_counters_inc(int, struct pf_pdesc *,
>   struct pf_state *, struct pf_rule *,
>   struct pf_rule *);
> @@ -3019,74 +3041,37 @@ pf_tag_packet(struct mbuf *m, int tag, i
>   m->m_pkthdr.ph_rtableid = (u_int)rtableid;
>  }
>  
> -void
> -pf_step_into_anchor(int *depth, struct pf_ruleset **rs,
> -struct pf_rule **r, struct pf_rule **a)
> +int
> +pf_step_into_anchor(struct pf_test_ctx *cx, struct pf_rule *r)
>  {
> - struct pf_anchor_stackframe *f;
> -
> - if (*depth >= sizeof(pf_anchor_stack) /
> - sizeof(pf_anchor_stack[0])) {
> - log(LOG_ERR, "pf: anchor stack overflow\n");
> - *r = TAILQ_NEXT(*r, entries);
> - return;
> - } else if (a != NULL)
> - *a = *r;
> - f = pf_anchor_stack + (*depth)++;
> - f->rs = *rs;
> - 

Re: RTC clocks

[Mark Kettenis]

> From: "Todd C. Miller" 
> Date: Sun, 26 Mar 2017 19:42:01 -0600
> 
> On Sun, 26 Mar 2017 23:25:48 +0200, Mark Kettenis wrote:
> 
> > The downside of this is that it becomes impossible to set the time
> > back that far.  But the upside is that if you haven't left a machine
> > powered off for a very long time, the ntpd constraint will actually
> > work.
> 
> I think that is a reasonable trade off.

Thanks.  I committed a slightly modified version since I thought a
comment on why the first year is rejected was appropriate.