distrib: arm64: remove gratitious customization from mr.fs

2021-02-14 Thread Sebastien Marie
Hi,

arm64 ramdisk has customization in mr.fs target, in order to create
usr/mdec/pine64 and usr/mdec/rpi directories (files will be copied
inside them by runlist.sh).

I will argue that mr.fs target isn't the place for such per-arch
customization as runlist.sh supports MKDIR directive from the list
file.

The diff belows put back mr.fs target identical to others archs and
make the directories to be created with directives from list file.

Please note that armv7 already uses such MKDIR directives for the same
purpose (usr/mdec/xxx directory creation).

Comments or OK ?
-- 
Sebastien Marie

diff 1c9ddf109f6795c0adfcf5b19fd0bc61b3839042 /home/semarie/repos/openbsd/src
blob - 04b89cbda8d32468e5e91d2785b43d8e6094d90b
file + distrib/arm64/ramdisk/Makefile
--- distrib/arm64/ramdisk/Makefile
+++ distrib/arm64/ramdisk/Makefile
@@ -24,10 +24,6 @@ UTILS=   ${.CURDIR}/../../miniroot
 MRFSDISKTYPE=  rdroot
 MRMAKEFSARGS=  -o disklabel=${MRFSDISKTYPE},minfree=0,density=4096
 
-DIRS=\
-   pine64 \
-   rpi
-
 PIFILES=\
bootcode.bin \
start.elf \
@@ -85,9 +81,6 @@ bsd:
 mr.fs: instbin
rm -rf $@.d
install -d -o root -g wheel $@.d
-.for DIR in ${DIRS}
-   mkdir -p $@.d/usr/mdec/${DIR}
-.endfor
mtree -def ${MTREE} -p $@.d -u
CURDIR=${.CURDIR} OBJDIR=${.OBJDIR} OSrev=${OSrev} \
TARGDIR=$@.d UTILS=${UTILS} RELEASEDIR=${RELEASEDIR} \
blob - 6e7a37085a9024988fd24d7d63a5689ea78aed4c
file + distrib/arm64/ramdisk/list
--- distrib/arm64/ramdisk/list
+++ distrib/arm64/ramdisk/list
@@ -91,6 +91,7 @@ COPY  ${DESTDIR}/etc/firmware/atu-rfmd2958-int etc/firm
 COPY   ${DESTDIR}/etc/firmware/atu-rfmd2958smc-ext 
etc/firmware/atu-rfmd2958smc-ext
 COPY   ${DESTDIR}/etc/firmware/atu-rfmd2958smc-int 
etc/firmware/atu-rfmd2958smc-int
 
+MKDIR  usr/mdec/rpi
 COPY   /usr/local/share/raspberrypi-firmware/boot/bcm2710-rpi-3-b.dtb 
usr/mdec/rpi/bcm2710-rpi-3-b.dtb
 COPY   /usr/local/share/raspberrypi-firmware/boot/bcm2710-rpi-3-b-plus.dtb 
usr/mdec/rpi/bcm2710-rpi-3-b-plus.dtb
 COPY   /usr/local/share/raspberrypi-firmware/boot/bcm2710-rpi-cm3.dtb 
usr/mdec/rpi/bcm2710-rpi-cm3.dtb
@@ -100,6 +101,7 @@ COPY
/usr/local/share/raspberrypi-firmware/boot/fixup.
 COPY   /usr/local/share/raspberrypi-firmware/boot/overlays/disable-bt.dtbo 
usr/mdec/rpi/disable-bt.dtbo
 COPY   /usr/local/share/u-boot/rpi_3/u-boot.bin usr/mdec/rpi/u-boot.bin
 
+MKDIR  usr/mdec/pine64
 COPY   /usr/local/share/u-boot/pine64_plus/u-boot-sunxi-with-spl.bin 
usr/mdec/pine64/u-boot-sunxi-with-spl.bin
 
 # copy the MAKEDEV script and make some devices



Re: distrib: make rdsetroot -x to work again

2021-02-14 Thread Sebastien Marie
On Sun, Feb 14, 2021 at 09:46:32AM -0700, Theo de Raadt wrote:
> Sure.
> 
> I guess the next thing to do would be using gzopen type functions,
> to edit a gzip'd version of the file.

I am unsure it would be doable in sane manner. rdsetroot is using
libelf to do elf manipulation, and the read-write support is using a
plain file descriptor.

and adding support of gz in libelf is a no for me.

gunzip the file before manipulation isn't a big deal.

> Then the recent move to .gz files would be more transparent for this
> specific use case (which I think is very rare, honestly).  I'm not sure
> if it matters.

I would not say it is rare, but I agree that it isn't a common use
case.

I recently asked on ports@ about withdraw upobsd tool I wrote (we have
sysupgrade in base now which do upgrade better than my tool), and I
got several replies from people using it for unattented installation.

Thanks.
-- 
Sebastien Marie



Re: distrib: make rdsetroot -x to work again

2021-02-14 Thread Sebastien Marie
On Sun, Feb 14, 2021 at 09:54:15AM -0500, Daniel Jakots wrote:
> On Sun, 14 Feb 2021 15:23:05 +0100, Sebastien Marie 
> wrote:
> 
> In the alpha diff, I would put the "-R .eh_frame -R .shstrtab \" line
> before the -K line so the -R things are grouped together.

I put it after in order to make more obvious it is a MD part (alpha is
the sole arch which remove such sections).

Some investigations:

-R .shstrtab was added in 2017

commit e8a1490d19dbdd7d68fc1a5e96fc7c5115e2c42a
from: deraadt 
date: Sun Sep 17 16:33:07 2017 UTC
 
 Some further shrinking, but obviously not enough.  Something unknown
 caused bloat about a month ago (and it wasn't purely the ctf additions
 since those are being stripped).  Maybe the compiler generates
 different code when stronger debugging information is requested?
 

-R .eh_frame was added in 2004

commit ec828d0bd39e2262e00f331f64c0a74a91cf0e0c
from: miod 
date: Fri Nov  5 21:22:37 2004 UTC
 
 Binutils 2.15 require more aggressive stripping for installation media 
binaries,
 if we want to still fit on floppies (binaries carry one extra section now,
 which we don't need on installation media).
 ok deraadt@


I would be interested to know the bytes gained by removing these
sections. We might removed them on others archs too.

Thanks.
-- 
Sebastien Marie



distrib: make rdsetroot -x to work again

2021-02-14 Thread Sebastien Marie
Hi,

The following diff makes rdsetroot -x (extract the disk.fs image) to
work again for stripped bsd.rd.

It passes options to keep rd_root_size and rd_root_image symbols while
stripping. These symbols are the ones used by rdsetroot to insert or
extract disk image into RAMDISK.

If it matter, on my i386 test, the bsd.rd size grows to 284 bytes
before gzip and 113 bytes after gzip.

While here, uniformize a bit the sections removed (.comment section
wasn't removed on some archs while stripping).

Comments or OK ?
-- 
Sebastien Marie

Index: distrib/alpha/miniroot/Makefile
===
RCS file: /cvs/src/distrib/alpha/miniroot/Makefile,v
retrieving revision 1.21
diff -u -p -r1.21 Makefile
--- distrib/alpha/miniroot/Makefile 13 Feb 2021 18:48:23 -  1.21
+++ distrib/alpha/miniroot/Makefile 14 Feb 2021 13:58:49 -
@@ -62,7 +62,10 @@ ${CDROM}: bsd.gz
rm -f vnd
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf -R .eh_frame -R .shstrtab bsd.rd 
bsd.strip
+   objcopy -S -R .comment -R .SUNW_ctf \
+   -K rd_root_size -K rd_root_image \
+   -R .eh_frame -R .shstrtab \
+   bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
 
 bsd.rd: mr.fs bsd
Index: distrib/amd64/ramdiskA/Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdiskA/Makefile,v
retrieving revision 1.14
diff -u -p -r1.14 Makefile
--- distrib/amd64/ramdiskA/Makefile 13 Feb 2021 18:52:08 -  1.14
+++ distrib/amd64/ramdiskA/Makefile 14 Feb 2021 13:58:49 -
@@ -33,7 +33,9 @@ MRDISKTYPE=   rdroot
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf bsd.rd bsd.strip
+   objcopy -S -R .comment -R .SUNW_ctf \
+   -K rd_root_size -K rd_root_image \
+   bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
 
 bsd.rd: mr.fs bsd
Index: distrib/amd64/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdisk_cd/Makefile,v
retrieving revision 1.28
diff -u -p -r1.28 Makefile
--- distrib/amd64/ramdisk_cd/Makefile   13 Feb 2021 18:52:08 -  1.28
+++ distrib/amd64/ramdisk_cd/Makefile   14 Feb 2021 13:58:49 -
@@ -56,7 +56,9 @@ MRDISKTYPE=   rdrootb
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf bsd.rd bsd.strip
+   objcopy -S -R .comment -R .SUNW_ctf \
+   -K rd_root_size -K rd_root_image \
+   bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
 
 bsd.rd: mr.fs bsd
Index: distrib/i386/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/i386/ramdisk/Makefile,v
retrieving revision 1.13
diff -u -p -r1.13 Makefile
--- distrib/i386/ramdisk/Makefile   13 Feb 2021 18:52:08 -  1.13
+++ distrib/i386/ramdisk/Makefile   14 Feb 2021 13:58:49 -
@@ -34,7 +34,9 @@ MRDISKTYPE=   rdroot
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf bsd.rd bsd.strip
+   objcopy -S -R .comment -R .SUNW_ctf \
+   -K rd_root_size -K rd_root_image \
+   bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
 
 bsd.rd: mr.fs bsd
Index: distrib/i386/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/i386/ramdisk_cd/Makefile,v
retrieving revision 1.22
diff -u -p -r1.22 Makefile
--- distrib/i386/ramdisk_cd/Makefile13 Feb 2021 18:52:08 -  1.22
+++ distrib/i386/ramdisk_cd/Makefile14 Feb 2021 13:58:49 -
@@ -53,7 +53,9 @@ MRDISKTYPE=   rdrootb
 MRMAKEFSARGS=  -o disklabel=${MRDISKTYPE},minfree=0,density=4096
 
 bsd.gz: bsd.rd
-   objcopy -S -R .comment -R .SUNW_ctf bsd.rd bsd.strip
+   objcopy -S -R .comment -R .SUNW_ctf \
+   -K rd_root_size -K rd_root_image \
+   bsd.rd bsd.strip
gzip -9cn bsd.strip > bsd.gz
 
 bsd.rd: mr.fs bsd
Index: distrib/macppc/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/macppc/ramdisk/Makefile,v
retrieving revision 1.48
diff -u -p -r1.48 Makefile
--- distrib/macppc/ramdisk/Makefile 5 Jan 2021 15:10:43 -   1.48
+++ distrib/macppc/ramdisk/Makefile 14 Feb 2021 13:58:49 -
@@ -35,9 +35,10 @@ bsd.gz: bsd.rd
gzip -9cn bsd.rd > bsd.gz
 
 bsd.rd: mr.fs bsd
-   cp bsd bsd.rd
+   objcopy -S -R .comment -R .SUNW_ctf \
+   -K rd_root_size -K rd_root_image \
+   bsd bsd.rd
rdsetroot bsd.rd mr.fs
-   strip -R .SUNW_ctf bsd.rd
 
 bsd:
cd ${.CURDIR}/../../../sys/arch/${MACHINE}/compile/${RAMDISK} && \
Index: distrib

distrib: reduce differences between archs: use ${MACHINE} when possible

2021-02-14 Thread Sebastien Marie
Hi,

The following diff makes various distrib Makefile to use ${MACHINE}
instead of hardcoded value.

Currently, some archs are using ${MACHINE} and others are using
hardcoded value.

Please note I am unsure about sgi: disklabel -w is using
"OpenBSD/sgi " as packid, and I changed it to "OpenBSD/${MACHINE}".
But the packid used isn't uniform across archs, so it might not matter.

The purpose is to reduce gradually the difference of Makefile between
archs.

Comments or OK ?
-- 
Sebastien Marie

Index: amd64/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/amd64/ramdisk_cd/Makefile,v
retrieving revision 1.28
diff -u -p -r1.28 Makefile
--- amd64/ramdisk_cd/Makefile   13 Feb 2021 18:52:08 -  1.28
+++ amd64/ramdisk_cd/Makefile   14 Feb 2021 13:40:13 -
@@ -38,18 +38,18 @@ ${FS}: bsd.gz
 
 ${CDROM}: bsd.rd
rm -rf ${.OBJDIR}/cd-dir
-   mkdir -p ${.OBJDIR}/cd-dir/${OSREV}/amd64
+   mkdir -p ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
mkdir -p ${.OBJDIR}/cd-dir/etc
-   echo "set image /${OSREV}/amd64/bsd.rd" > 
${.OBJDIR}/cd-dir/etc/boot.conf
-   cp ${.OBJDIR}/bsd.rd ${.OBJDIR}/cd-dir/${OSREV}/amd64
-   cp ${DESTDIR}/usr/mdec/cdbr ${.OBJDIR}/cd-dir/${OSREV}/amd64
-   cp ${DESTDIR}/usr/mdec/cdboot ${.OBJDIR}/cd-dir/${OSREV}/amd64/cdboot
+   echo "set image /${OSREV}/${MACHINE}/bsd.rd" > 
${.OBJDIR}/cd-dir/etc/boot.conf
+   cp ${.OBJDIR}/bsd.rd ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
+   cp ${DESTDIR}/usr/mdec/cdbr ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
+   cp ${DESTDIR}/usr/mdec/cdboot 
${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}/cdboot
mkhybrid -a -R -T -L -l -d -D -N -o ${.OBJDIR}/${CDROM} \
-   -A "OpenBSD ${OSREV} amd64 bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-   -V "OpenBSD/amd64   ${OSREV} boot-only CD" \
-   -b ${OSREV}/amd64/cdbr -c ${OSREV}/amd64/boot.catalog \
+   -V "OpenBSD/${MACHINE}   ${OSREV} boot-only CD" \
+   -b ${OSREV}/${MACHINE}/cdbr -c ${OSREV}/${MACHINE}/boot.catalog \
${.OBJDIR}/cd-dir
 
 MRDISKTYPE=rdrootb
Index: hppa/ramdisk/Makefile
===
RCS file: /cvs/src/distrib/hppa/ramdisk/Makefile,v
retrieving revision 1.46
diff -u -p -r1.46 Makefile
--- hppa/ramdisk/Makefile   17 May 2020 17:04:27 -  1.46
+++ hppa/ramdisk/Makefile   14 Feb 2021 13:40:13 -
@@ -20,10 +20,10 @@ ${CDROM}: bsd.rd
rm -rf ${.OBJDIR}/cd-dir/
mkdir -p ${.OBJDIR}/cd-dir/
cp bsd.rd ${.OBJDIR}/cd-dir/bsd.rd
-   mkhybrid -A "OpenBSD ${OSREV} hppa bootonly CD" \
+   mkhybrid -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-   -V "OpenBSD/hppa ${OSREV} boot-only CD" \
+   -V "OpenBSD/${MACHINE} ${OSREV} boot-only CD" \
-o ${.OBJDIR}/${CDROM} ${.OBJDIR}/cd-dir
dd if=${DESTDIR}/usr/mdec/cdboot of=${.OBJDIR}/${CDROM} \
bs=32k count=1 conv=notrunc
Index: i386/ramdisk_cd/Makefile
===
RCS file: /cvs/src/distrib/i386/ramdisk_cd/Makefile,v
retrieving revision 1.22
diff -u -p -r1.22 Makefile
--- i386/ramdisk_cd/Makefile13 Feb 2021 18:52:08 -  1.22
+++ i386/ramdisk_cd/Makefile14 Feb 2021 13:40:13 -
@@ -35,18 +35,18 @@ ${FS}: bsd.gz
 
 ${CDROM}: bsd.rd
rm -rf ${.OBJDIR}/cd-dir
-   mkdir -p ${.OBJDIR}/cd-dir/${OSREV}/i386
+   mkdir -p ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
mkdir -p ${.OBJDIR}/cd-dir/etc
-   echo "set image /${OSREV}/i386/bsd.rd" > ${.OBJDIR}/cd-dir/etc/boot.conf
-   cp ${.OBJDIR}/bsd.rd ${.OBJDIR}/cd-dir/${OSREV}/i386
-   cp ${DESTDIR}/usr/mdec/cdbr ${.OBJDIR}/cd-dir/${OSREV}/i386
-   cp ${DESTDIR}/usr/mdec/cdboot ${.OBJDIR}/cd-dir/${OSREV}/i386/cdboot
+   echo "set image /${OSREV}/${MACHINE}/bsd.rd" > 
${.OBJDIR}/cd-dir/etc/boot.conf
+   cp ${.OBJDIR}/bsd.rd ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
+   cp ${DESTDIR}/usr/mdec/cdbr ${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}
+   cp ${DESTDIR}/usr/mdec/cdboot 
${.OBJDIR}/cd-dir/${OSREV}/${MACHINE}/cdboot
mkhybrid -a -R -T -L -l -d -D -N -o ${.OBJDIR}/${CDROM} \
-   -A "OpenBSD ${OSREV} i386 bootonly CD" \
+   -A "OpenBSD ${OSREV} ${MACHINE} bootonly CD" \
-P "Copyright (c) `date +%Y` Theo de Raadt, The OpenBSD project" \
-p "Theo de Raadt " \
-   -V "OpenBSD/

Re: [diff] src/usr.sbin/smtpd: add a forward-file option

2020-12-20 Thread Sebastien Marie
On Sat, Dec 19, 2020 at 11:19:10PM -0700, Theo de Raadt wrote:
> There are thousands of people with smtpd configurations, and sysmerge
> is not going to handle this.
> 
> We cannot expect them all to change their files.  This is madness.

Well, it wouldn't be the first time. But I agree that such changes
should be rare and have really good reason for.

So yes, even if the option is desirable and being off-by-default would
be a good default, the flag-day way for handling it is complex.


Regarding the option itself, if I recall correctly some descriptions
made by Gilles about smtpd, opening ~/.forward is one of the few tasks
done by the priviligied process of smtpd. So it could make sense to
avoid it if not need.

Gilles, could you confirm that having an option to remove .forward
capability (whatever the default value of the option is) could
effectively help to reduce the attack surface of smtpd ?

For example, as immediate consequence, I see no reason for smtpd
priviliegied process to keep a full filesystem view: it might be
possible to restricted it to few directories with unveil(2) (I assume
priviliegied process is still need for bsd_auth, and bsd_auth will
have some requirements).

Thanks.
-- 
Sebastien Marie



Re: [diff] src/usr.sbin/smtpd: add a forward-file option

2020-12-19 Thread Sebastien Marie
On Sat, Dec 19, 2020 at 10:36:32PM +, gil...@poolp.org wrote:
> Hello,
> 
> Whenever a rule with a local action (mbox, maildir, lmtp or mda) is matched, 
> smtpd will
> attempt to search for a ~/.forward file in the recipient directory and 
> process it. This
> may be convenient for some setups but it is an implicit behavior that's not 
> overridable
> and not always wanted.
> 
> This diff changes this behavior by requiring the admins to explicitly allow 
> the forward
> files processing in the actions when desired:
> 
> action "local_users" maildir forward-file
> 
> 
> With this diff, if forward-file is not specified, code to request parent 
> process for an
> fd is bypassed and the expansion layer just pretends parent couldn't find 
> one. This let
> the code fallback in an already existing code path with the proper behavior 
> and is very
> uninvasive.
> 

if I could understood the direction (which is fine as it makes the
daemon less behaviour dependant on a user settings), the default seems
wrong to me (at least for now, and for OpenBSD base specifically).

Currently, root@ mail delivery is based on /root/.forward file:
install is writing this file to redirect root@ mail to user (if user
was created at install-time). It is done this way since 2011 (see
distrib/miniroot/install.sh rev 1.218). So I assume that all installs
which were done with a user configured, since 2011, could use it.

At first step, I would keep the default smtpd.conf with "forward-file"
option set. It would make smtpd(1) to default to no "forward-file" if
not set (what your diff do), but set the default to with
"forward-file" for OpenBSD base.

Admin could remove the option if he/she doesn't use it.

Thanks.
-- 
Sebastien Marie



Re: wireguard + witness

2020-12-01 Thread Sebastien Marie
On Tue, Dec 01, 2020 at 06:59:22AM +0100, Sebastien Marie wrote:
> On Mon, Nov 30, 2020 at 11:14:46PM +, Stuart Henderson wrote:
> > Thought I'd try a WITNESS kernel to see if that gives any clues about
> > what's going on with my APU crashing all over the place (long shot but
> > I got bored with trying different older kernels..)
> > 
> > I see these from time to time (one during netstart, and another 4 in
> > 15 mins uptime), anyone know if they're important?
> 
> this check ("lock_object uninitialized") was recently added in witness.
> 
> it means that the rwlock was uninitialized (the witness flag
> LO_INITIALIZED isn't present whereas it should) when witness check the
> lock.
> 
> it could be:
> - someone omits to call rw_init() or RWLOCK_INITIALIZER() (possible bug if 
> memory isn't zero)
> - the struct has been zeroed (possible bug too)
> 
> > witness: lock_object uninitialized: 0x80bcf0d8
> > Starting stack trace...
> > witness_checkorder(80bcf0d8,9,0) at witness_checkorder+0xab
> > rw_enter_write(80bcf0c8) at rw_enter_write+0x43
> > noise_remote_decrypt(80bcea48,978dc3d,0,fd805e22cb7c,10) at 
> > noise_remote_decrypt+0x135
> > wg_decap(8054a000,fd8061835200) at wg_decap+0xda
> > wg_decap_worker(8054a000) at wg_decap_worker+0x7a
> > taskq_thread(8012d900) at taskq_thread+0x9f
> > end trace frame: 0x0, count: 251
> > End of stack trace.
> 
> from the trace, the sole rw_enter_write() usage in noise_remote_decrypt() is
> 
>  rw_enter_write(>r_keypair_lock)
> 
> but I am seeing few rw_init() on r_keypair_lock. I will see if I could
> see the source of the problem.
> 

Jason, Matt,

sthen@ told me that the same lock is reported several times (exactly,
two locks are reported several times: lock1, lock2, lock1, lock2...)

witness(4) reports when a lock doesn't have LO_INITIALIZED flag set in
internal part of `struct rwlock'. Next it sets it, and skip futher
analysis for this particular lock.

if the same lock is reported several times, it means the memory is
zeroed (and so the flag removed). it could be intented or not. but in
all cases, a rwlock should be properly initialized with rw_init() or
RWLOCK_INITIALIZER() before use.

I don't have enough experiency with wg(4) stuff to well understand the
issue. in wg_decap(), the peer is get from a `struct wg_tag` (from
wg_tag_get()). If i didn't mess myself, the p_remote could come from
wg_send_initiation() or wg_send_response(). but for both, it comes
from wg_peer_create(), and p_remote is initialized with
noise_remote_init() (and so lock have proper rw_init()).

do you have idea on the cause of the lost of the rwlock flag ?

if you want to test it with witness(4) enabled, you will need to build
a kernel with "option WITNESS" in config file. you could uncomment it
from sys/arch/amd64/conf/GENERIC.MP, and run make config, make clean,
make

Thanks.
-- 
Sebastien Marie



Re: wireguard + witness

2020-11-30 Thread Sebastien Marie
On Mon, Nov 30, 2020 at 11:14:46PM +, Stuart Henderson wrote:
> Thought I'd try a WITNESS kernel to see if that gives any clues about
> what's going on with my APU crashing all over the place (long shot but
> I got bored with trying different older kernels..)
> 
> I see these from time to time (one during netstart, and another 4 in
> 15 mins uptime), anyone know if they're important?

this check ("lock_object uninitialized") was recently added in witness.

it means that the rwlock was uninitialized (the witness flag
LO_INITIALIZED isn't present whereas it should) when witness check the
lock.

it could be:
- someone omits to call rw_init() or RWLOCK_INITIALIZER() (possible bug if 
memory isn't zero)
- the struct has been zeroed (possible bug too)

> witness: lock_object uninitialized: 0x80bcf0d8
> Starting stack trace...
> witness_checkorder(80bcf0d8,9,0) at witness_checkorder+0xab
> rw_enter_write(80bcf0c8) at rw_enter_write+0x43
> noise_remote_decrypt(80bcea48,978dc3d,0,fd805e22cb7c,10) at 
> noise_remote_decrypt+0x135
> wg_decap(8054a000,fd8061835200) at wg_decap+0xda
> wg_decap_worker(8054a000) at wg_decap_worker+0x7a
> taskq_thread(8012d900) at taskq_thread+0x9f
> end trace frame: 0x0, count: 251
> End of stack trace.

from the trace, the sole rw_enter_write() usage in noise_remote_decrypt() is

 rw_enter_write(>r_keypair_lock)

but I am seeing few rw_init() on r_keypair_lock. I will see if I could
see the source of the problem.

thanks.
-- 
Sebastien Marie



Re: myx(4): add initialization of sc_sff_lock rwlock

2020-11-26 Thread Sebastien Marie
On Thu, Nov 26, 2020 at 02:54:11PM +0800, Kevin Lo wrote:
> Ok?

yes please.

ok semarie@

> Index: sys/dev/pci/if_myx.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_myx.c,v
> retrieving revision 1.111
> diff -u -p -u -p -r1.111 if_myx.c
> --- sys/dev/pci/if_myx.c  17 Jul 2020 03:37:36 -  1.111
> +++ sys/dev/pci/if_myx.c  26 Nov 2020 06:49:04 -
> @@ -270,6 +270,8 @@ myx_attach(struct device *parent, struct
>   char part[32];
>   pcireg_t memtype;
>  
> + rw_init(>sc_sff_lock, "myxsff");
> +
>   sc->sc_pc = pa->pa_pc;
>   sc->sc_tag = pa->pa_tag;
>   sc->sc_dmat = pa->pa_dmat;
> 

-- 
Sebastien Marie



Re: drm: avoid possible deadlock in kthread_stop

2020-10-17 Thread Sebastien Marie
On Wed, Oct 14, 2020 at 08:58:04PM +0200, Mark Kettenis wrote:
> > Date: Thu, 1 Oct 2020 09:09:50 +0200
> > From: Sebastien Marie 
> > 
> > Hi,
> > 
> > Currently, when a process is calling kthread_stop(), it sets a flag
> > asking the thread to stop, and enters in sleep mode, but the code
> > doing the stop doesn't wakeup the caller of kthread_stop().
> > 
> > The thread should also be unparked as else it will not seen the
> > KTHREAD_SHOULDSTOP flag. it follows what Linux is doing.
> > 
> > While here, I added some comments in the locking logic for park/unpark
> > and stop.
> > 
> > Comments or OK ?
> 
> I don't think adding all those comments makes a lot of sense.  This
> uses a fairly standard tsleep/wakeup pattern and the some of the
> comments really state the obvious.

it was the way I did to audit the code and understand what it did.

> Can you do a diff that just adds
> the missing wakeup() and kthread_unpark() call?

here a new diff.

diff 4efbe95c75086b3a7b0074651bfa04fd58990a98 /home/semarie/repos/openbsd/src
blob - fd797effc74d6eb4a172c81be8feac0ed168ec5d
file + sys/dev/pci/drm/drm_linux.c
--- sys/dev/pci/drm/drm_linux.c
+++ sys/dev/pci/drm/drm_linux.c
@@ -207,6 +217,7 @@ kthread_func(void *arg)
 
ret = thread->func(thread->data);
thread->flags |= KTHREAD_STOPPED;
+   wakeup(thread);
kthread_exit(ret);
 }
 
@@ -298,8 +327,9 @@ kthread_stop(struct proc *p)
 
while ((thread->flags & KTHREAD_STOPPED) == 0) {
thread->flags |= KTHREAD_SHOULDSTOP;
+   kthread_unpark(p);
wake_up_process(thread->proc);
tsleep_nsec(thread, PPAUSE, "stop", INFSLP);
}
LIST_REMOVE(thread, next);
 

Thanks.
-- 
Sebastien Marie



Re: amap: KASSERT()s and local variables

2020-10-11 Thread Sebastien Marie
On Wed, Oct 07, 2020 at 04:49:28PM +0200, Martin Pieuchot wrote:
> On 01/10/20(Thu) 14:18, Martin Pieuchot wrote:
> > Use more KASSERT()s instead of the "if (x) panic()" idiom for sanity
> > checks and add a couple of local variables to reduce the difference
> > with NetBSD and help for upcoming locking.
> 
> deraadt@ mentioned that KASSERT()s are not effective in RAMDISK kernels.
> 
> So the revisited diff below only converts checks that are redundant with
> NULL dereferences.
> 
> ok?

globally ok. but there is one problem, see below.

> Index: uvm/uvm_amap.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 uvm_amap.c
> --- uvm/uvm_amap.c25 Sep 2020 08:04:48 -  1.84
> +++ uvm/uvm_amap.c7 Oct 2020 14:40:53 -
> @@ -669,9 +669,7 @@ ReStart:
>   pg = anon->an_page;
>  
>   /* page must be resident since parent is wired */
> - if (pg == NULL)
> - panic("amap_cow_now: non-resident wired page"
> - " in anon %p", anon);
> + KASSERT(pg != NULL);
>  
>   /*
>* if the anon ref count is one, we are safe (the child
> @@ -740,6 +738,7 @@ ReStart:
>  void
>  amap_splitref(struct vm_aref *origref, struct vm_aref *splitref, vaddr_t 
> offset)
>  {
> + struct vm_amap *amap = origref->ar_amap;
>   int leftslots;
>  
>   AMAP_B2SLOT(leftslots, offset);
> @@ -747,17 +746,18 @@ amap_splitref(struct vm_aref *origref, s
>   panic("amap_splitref: split at zero offset");
>  
>   /* now: we have a valid am_mapped array. */
> - if (origref->ar_amap->am_nslot - origref->ar_pageoff - leftslots <= 0)
> + if (amap->am_nslot - origref->ar_pageoff - leftslots <= 0)
>   panic("amap_splitref: map size check failed");
>  
>  #ifdef UVM_AMAP_PPREF
> -/* establish ppref before we add a duplicate reference to the amap */
> - if (origref->ar_amap->am_ppref == NULL)
> - amap_pp_establish(origref->ar_amap);
> +/* Establish ppref before we add a duplicate reference to the amap. 
> */
> + if (amap->am_ppref == NULL)
> + amap_pp_establish(amap);
>  #endif
>  
> - splitref->ar_amap = origref->ar_amap;
> - splitref->ar_amap->am_ref++;/* not a share reference */
> + /* Note: not a share reference. */
> + amap->am_ref++;
> + splitref->ar_amap = amap;
>   splitref->ar_pageoff = origref->ar_pageoff + leftslots;
>  }
>  
> @@ -1104,12 +1104,11 @@ amap_add(struct vm_aref *aref, vaddr_t o
>  
>   slot = UVM_AMAP_SLOTIDX(slot);
>   if (replace) {
> - if (chunk->ac_anon[slot] == NULL)
> - panic("amap_add: replacing null anon");
> - if (chunk->ac_anon[slot]->an_page != NULL &&
> - (amap->am_flags & AMAP_SHARED) != 0) {
> - pmap_page_protect(chunk->ac_anon[slot]->an_page,
> - PROT_NONE);
> + struct vm_anon *oanon  = chunk->ac_anon[slot];
> +
> + KASSERT(oanon != NULL);
> + if (oanon->an_page && (amap->am_flags & AMAP_SHARED) != 0) {
> + pmap_page_protect(oanon->an_page, PROT_NONE);
>   /*
>* XXX: suppose page is supposed to be wired somewhere?
>*/
> @@ -1138,14 +1137,13 @@ amap_unadd(struct vm_aref *aref, vaddr_t
>  
>   AMAP_B2SLOT(slot, offset);
>   slot += aref->ar_pageoff;
> - KASSERT(slot < amap->am_nslot);
> + if (chunk->ac_anon[slot] == NULL)
> + panic("amap_unadd: nothing there");

this change seems wrong. you're removing a KASSERT() and readd an
explicit panic(9) (taken from few lines after)

>   chunk = amap_chunk_get(amap, slot, 0, PR_NOWAIT);
> - if (chunk == NULL)
> - panic("amap_unadd: chunk for slot %d not present", slot);
> + KASSERT(chunk != NULL);
>  
>   slot = UVM_AMAP_SLOTIDX(slot);
> - if (chunk->ac_anon[slot] == NULL)
> - panic("amap_unadd: nothing there");
> + KASSERT(chunk->ac_anon[slot] != NULL);
>  
>   chunk->ac_anon[slot] = NULL;
>   chunk->ac_usedmap &= ~(1 << slot);
> 

Thanks.
-- 
Sebastien Marie



drm: avoid possible deadlock in kthread_stop

2020-10-01 Thread Sebastien Marie
Hi,

Currently, when a process is calling kthread_stop(), it sets a flag
asking the thread to stop, and enters in sleep mode, but the code
doing the stop doesn't wakeup the caller of kthread_stop().

The thread should also be unparked as else it will not seen the
KTHREAD_SHOULDSTOP flag. it follows what Linux is doing.

While here, I added some comments in the locking logic for park/unpark
and stop.

Comments or OK ?

Thanks.
-- 
Sebastien Marie

---
commit 70e71461c8598e28820f1743923cac40670f7c33
from: Sébastien Marie 
date: Thu Oct  1 07:02:46 2020 UTC
 
 properly support kthread_stop()
 - wakeup pthread_stop() caller
 - unpark the thread if parked
 
 while here, add comments for locking logic for park/unpark/stop
 
diff ec329a4429e2542bc24dd017b8001b22df43564c 
ce2b5031503711bbdd7a3067c76c4f18b1d8da82
blob - 2cbd0905406ccc9d89c86cee38673a4e9c3fcf42
blob + f0e5a5a1b282c071c97505556510952ee7a6282a
--- sys/dev/pci/drm/drm_linux.c
+++ sys/dev/pci/drm/drm_linux.c
@@ -206,6 +206,10 @@ kthread_func(void *arg)
 
ret = thread->func(thread->data);
thread->flags |= KTHREAD_STOPPED;
+
+   /* wakeup thread waiting in kthread_stop() */
+   wakeup(thread);
+
kthread_exit(ret);
 }
 
@@ -256,7 +260,14 @@ kthread_parkme(void)
 
while (thread->flags & KTHREAD_SHOULDPARK) {
thread->flags |= KTHREAD_PARKED;
+
+   /* 
+* wakeup kthread_park() caller
+* to signal I am parked as asked.
+*/
wakeup(thread);
+
+   /* wait for someone to kthread_unpark() me */
tsleep_nsec(thread, PPAUSE, "parkme", INFSLP);
thread->flags &= ~KTHREAD_PARKED;
}
@@ -269,7 +280,13 @@ kthread_park(struct proc *p)
 
while ((thread->flags & KTHREAD_PARKED) == 0) {
thread->flags |= KTHREAD_SHOULDPARK;
+
wake_up_process(thread->proc);
+
+   /*
+* wait for thread to be parked.
+* the asked thread should call kthread_parkme()
+*/
tsleep_nsec(thread, PPAUSE, "park", INFSLP);
}
 }
@@ -280,6 +297,8 @@ kthread_unpark(struct proc *p)
struct kthread *thread = kthread_lookup(p);
 
thread->flags &= ~KTHREAD_SHOULDPARK;
+
+   /* wakeup kthread_parkme() caller */
wakeup(thread);
 }
 
@@ -297,7 +316,13 @@ kthread_stop(struct proc *p)
 
while ((thread->flags & KTHREAD_STOPPED) == 0) {
thread->flags |= KTHREAD_SHOULDSTOP;
+
+   /* kthread_unpark() the thread if parked */
+   kthread_unpark(p);
+
wake_up_process(thread->proc);
+   
+   /* wait for thread to stop (func() should return) */
tsleep_nsec(thread, PPAUSE, "stop", INFSLP);
}
LIST_REMOVE(thread, next);




Re: btrace: add boolean AND and OR operators

2020-09-14 Thread Sebastien Marie
ort("invalid argument type %d", ba->ba_type);
> Index: regress/usr.sbin/btrace/Makefile
> ===
> RCS file: /cvs/src/regress/usr.sbin/btrace/Makefile,v
> retrieving revision 1.4
> diff -u -p -r1.4 Makefile
> --- regress/usr.sbin/btrace/Makefile  19 Mar 2020 15:53:09 -  1.4
> +++ regress/usr.sbin/btrace/Makefile  14 Sep 2020 15:14:10 -
> @@ -3,8 +3,8 @@
>  BTRACE?=  /usr/sbin/btrace
>  
>  # scripts that don't need /dev/dt
> -BT_LANG_SCRIPTS= arithm beginend comments delete exit map map-unnamed \
> - maxoperand min+max+sum multismts nsecs+var
> +BT_LANG_SCRIPTS= arithm beginend boolean comments delete exit map \
> + map-unnamed maxoperand min+max+sum multismts nsecs+var
>  
>  BT_KERN_SCRIPTS=
>  
> Index: regress/usr.sbin/btrace/boolean.bt
> ===
> RCS file: regress/usr.sbin/btrace/boolean.bt
> diff -N regress/usr.sbin/btrace/boolean.bt
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ regress/usr.sbin/btrace/boolean.bt14 Sep 2020 15:14:10 -
> @@ -0,0 +1,8 @@
> +BEGIN
> +{
> + @a = 9;
> + @b = 1;
> +
> + printf("a & b = %d\n", @a & @b);
> + printf("a | b = %d\n", @a | @b);
> +}
> Index: regress/usr.sbin/btrace/boolean.ok
> ===
> RCS file: regress/usr.sbin/btrace/boolean.ok
> diff -N regress/usr.sbin/btrace/boolean.ok
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ regress/usr.sbin/btrace/boolean.ok14 Sep 2020 15:14:10 -
> @@ -0,0 +1,2 @@
> +a & b = 1
> +a | b = 9
> -- 
> jasper
> 

-- 
Sebastien Marie



Re: go/rust vs uvm_map_inentry()

2020-09-14 Thread Sebastien Marie
On Mon, Sep 14, 2020 at 01:25:03PM +0200, Mark Kettenis wrote:
> > Date: Sun, 13 Sep 2020 19:48:19 +0200
> > From: Sebastien Marie 
> > 
> > On Sun, Sep 13, 2020 at 04:49:48PM +0200, Sebastien Marie wrote:
> > > On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > > > I'm no longer able to reproduce the corruption while building lang/go
> > > > with the diff below.  Something relevant to threading change in go since
> > > > march?
> > > > 
> > > > Can someone try this diff and tell me if go and/or rust still fail?
> > > 
> > > quickly tested with rustc build (nightly here), and it is failing at 
> > > random places (not always at the same) with memory errors (signal 11, 
> > > compiler ICE signal 6...)
> > > 
> > 
> > A first hint.
> > 
> > With the help of deraadt@, it was found that disabling
> > uvm_map_inentry() call in usertrap() is enough to avoid the crashes.
> > 
> > To be clear, I am using the following diff:
> 
> The diff below fixes at (for amd64).
> 
> What's happening is that uvm_map_inentry() may sleep to grab the lock
> of the map.  The fault address is read from cr2 in pageflttrap() which
> gets called after this check and if the check sleeps, cr2 is likely to
> be clobbered by a page fault in another process.
> 
> Diff below fixes this by reading cr2 early and passing it to pageflttrap().
> 
> ok?

it makes sens. and I can't trigger the crashes with rustc build anymore.

ok semarie@
 
> 
> Index: arch/amd64/amd64/trap.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 trap.c
> --- arch/amd64/amd64/trap.c   19 Aug 2020 10:10:57 -  1.80
> +++ arch/amd64/amd64/trap.c   14 Sep 2020 11:17:35 -
> @@ -92,7 +92,7 @@
>  
>  #include "isa.h"
>  
> -int  pageflttrap(struct trapframe *, int _usermode);
> +int  pageflttrap(struct trapframe *, uint64_t, int _usermode);
>  void kerntrap(struct trapframe *);
>  void usertrap(struct trapframe *);
>  void ast(struct trapframe *);
> @@ -157,12 +157,11 @@ fault(const char *format, ...)
>   * if something was so broken that we should panic.
>   */
>  int
> -pageflttrap(struct trapframe *frame, int usermode)
> +pageflttrap(struct trapframe *frame, uint64_t cr2, int usermode)
>  {
>   struct proc *p = curproc;
>   struct pcb *pcb;
>   int error;
> - uint64_t cr2;
>   vaddr_t va;
>   struct vm_map *map;
>   vm_prot_t ftype;
> @@ -172,7 +171,6 @@ pageflttrap(struct trapframe *frame, int
>  
>   map = >p_vmspace->vm_map;
>   pcb = >p_addr->u_pcb;
> - cr2 = rcr2();
>   va = trunc_page((vaddr_t)cr2);
>  
>   KERNEL_LOCK();
> @@ -280,6 +278,7 @@ void
>  kerntrap(struct trapframe *frame)
>  {
>   int type = (int)frame->tf_trapno;
> + uint64_t cr2 = rcr2();
>  
>   verify_smap(__func__);
>   uvmexp.traps++;
> @@ -299,7 +298,7 @@ kerntrap(struct trapframe *frame)
>   /*NOTREACHED*/
>  
>   case T_PAGEFLT: /* allow page faults in kernel mode */
> - if (pageflttrap(frame, 0))
> + if (pageflttrap(frame, cr2, 0))
>   return;
>   goto we_re_toast;
>  
> @@ -333,6 +332,7 @@ usertrap(struct trapframe *frame)
>  {
>   struct proc *p = curproc;
>   int type = (int)frame->tf_trapno;
> + uint64_t cr2 = rcr2();
>   union sigval sv;
>   int sig, code;
>  
> @@ -381,7 +381,7 @@ usertrap(struct trapframe *frame)
>   break;
>  
>   case T_PAGEFLT: /* page fault */
> - if (pageflttrap(frame, 1))
> + if (pageflttrap(frame, cr2, 1))
>   goto out;
>   /* FALLTHROUGH */
>  

-- 
Sebastien Marie



Re: go/rust vs uvm_map_inentry()

2020-09-13 Thread Sebastien Marie
On Sun, Sep 13, 2020 at 04:49:48PM +0200, Sebastien Marie wrote:
> On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> > I'm no longer able to reproduce the corruption while building lang/go
> > with the diff below.  Something relevant to threading change in go since
> > march?
> > 
> > Can someone try this diff and tell me if go and/or rust still fail?
> 
> quickly tested with rustc build (nightly here), and it is failing at random 
> places (not always at the same) with memory errors (signal 11, compiler ICE 
> signal 6...)
> 

A first hint.

With the help of deraadt@, it was found that disabling
uvm_map_inentry() call in usertrap() is enough to avoid the crashes.

To be clear, I am using the following diff:

diff 3e16148d8fe176d83ff415f6c03a79618da4401e /data/semarie/repos/openbsd/src
blob - 7f195a5309280943e0138953c61fffcb6a80c6bf
file + sys/arch/amd64/conf/GENERIC.MP
--- sys/arch/amd64/conf/GENERIC.MP
+++ sys/arch/amd64/conf/GENERIC.MP
@@ -4,6 +4,8 @@ include "arch/amd64/conf/GENERIC"
 
 option MULTIPROCESSOR
 #optionMP_LOCKDEBUG
-#optionWITNESS
+option WITNESS
+
+pseudo-device dt
 
 cpu*   at mainbus?
blob - fc23bc67e305a1a1edc7d6f08ecb982dccdc4a45
file + sys/uvm/uvm_map.c
--- sys/uvm/uvm_map.c
+++ sys/uvm/uvm_map.c
@@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p_inentry *ie, 
boolean_t ok = TRUE;
 
if (uvm_map_inentry_recheck(serial, addr, ie)) {
-   KERNEL_LOCK();
ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
if (!ok) {
+   KERNEL_LOCK();
printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
addr, ie->ie_start, ie->ie_end);
p->p_p->ps_acflag |= AMAP;
sv.sival_ptr = (void *)PROC_PC(p);
trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
+   KERNEL_UNLOCK();
}
-   KERNEL_UNLOCK();
}
return (ok);
 }
blob - 4a4c6275aa766fe2e4f5c9d913d1257f41a9d578
file + sys/arch/amd64/amd64/trap.c
--- sys/arch/amd64/amd64/trap.c
+++ sys/arch/amd64/amd64/trap.c
@@ -343,10 +343,12 @@ usertrap(struct trapframe *frame)
p->p_md.md_regs = frame;
refreshcreds(p);
 
+#if 0
if (!uvm_map_inentry(p, >p_spinentry, PROC_STACK(p),
"[%s]%d/%d sp=%lx inside %lx-%lx: not MAP_STACK\n",
uvm_map_inentry_sp, p->p_vmspace->vm_map.sserial))
goto out;
+#endif
 
    switch (type) {
case T_PROTFLT: /* protection fault */


Thanks.
-- 
Sebastien Marie



Re: go/rust vs uvm_map_inentry()

2020-09-13 Thread Sebastien Marie
On Sun, Sep 13, 2020 at 09:15:15AM -0600, Theo de Raadt wrote:
> crashes -- but without any kernel printfs?

crashes and no kernel printfs

-- 
Sebastien Marie



Re: go/rust vs uvm_map_inentry()

2020-09-13 Thread Sebastien Marie
On Sun, Sep 13, 2020 at 03:29:57PM +0200, Martin Pieuchot wrote:
> I'm no longer able to reproduce the corruption while building lang/go
> with the diff below.  Something relevant to threading change in go since
> march?
> 
> Can someone try this diff and tell me if go and/or rust still fail?

quickly tested with rustc build (nightly here), and it is failing at random 
places (not always at the same) with memory errors (signal 11, compiler ICE 
signal 6...)


> Index: uvm/uvm_map.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.266
> diff -u -p -r1.266 uvm_map.c
> --- uvm/uvm_map.c 12 Sep 2020 17:08:50 -  1.266
> +++ uvm/uvm_map.c 13 Sep 2020 10:12:25 -
> @@ -1893,16 +1893,16 @@ uvm_map_inentry(struct proc *p, struct p
>   boolean_t ok = TRUE;
>  
>   if (uvm_map_inentry_recheck(serial, addr, ie)) {
> - KERNEL_LOCK();
>   ok = uvm_map_inentry_fix(p, ie, addr, fn, serial);
>   if (!ok) {
> + KERNEL_LOCK();
>   printf(fmt, p->p_p->ps_comm, p->p_p->ps_pid, p->p_tid,
>   addr, ie->ie_start, ie->ie_end);
>   p->p_p->ps_acflag |= AMAP;
>   sv.sival_ptr = (void *)PROC_PC(p);
>   trapsignal(p, SIGSEGV, 0, SEGV_ACCERR, sv);
> + KERNEL_UNLOCK();
>   }
> - KERNEL_UNLOCK();
>   }
>   return (ok);
>  }
> 

-- 
Sebastien Marie



Re: who(1) patch for unveil violation

2020-08-27 Thread Sebastien Marie
On Thu, Aug 27, 2020 at 07:00:22AM -0400, David Goerger wrote:
> Hello,
> 
> This morning I was surprised to see a who(1) unveil violation in a
> lastcomm(1) report, so I looked into it and found that when requesting
> show_idle (-u flag) or show_term (-T flag), we indeed try to read
> _PATH_DEV, which isn't unveiled yet.
> 
> I'm not an unveil(2) expert, and there might be a better way to handle
> this, but I confirmed this fixes both case 0 (no file arg) and case 1
> (e.g. `who -u /var/log/wtmp`). Tested on a -current snapshot from
> yesterday, as well as on an up-to-date 6.7-stable box.
> 
> Cheers,
> David

The diff is ok semarie@

who(1) is doing stat(2) on line to determine +/- mode of the tty (for
show_term) or to determine the idle time using st_atime (show_idle).

> ===
> --- who.c.orig  Thu Aug 27 06:24:18 2020
> +++ who.c   Thu Aug 27 06:40:52 2020
> @@ -124,6 +124,10 @@
> 
> if (unveil(_PATH_UTMP, "r") == -1)
> err(1, "unveil");
> +   if (show_term || show_idle) {
> +   if (unveil(_PATH_DEV, "r") == -1)
> +   err(1, "unveil");
> +   }
> switch (argc) {
> case 0:     /* who */
> if (pledge("stdio rpath getpw", NULL) == -1)
> 

-- 
Sebastien Marie



Re: smtpd: make smarthost to use SNI when relaying

2020-05-31 Thread Sebastien Marie
Hi,

updated diff after millert@ and beck@ remarks:
- use union to collapse in_addr + in6_addr
- doesn't allocate buffer and directly use s->relay->domain->name

Thanks.
-- 
Sebastien Marie


diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
blob - d384692a0e43de47d645142a6b99e72b7d83b687
file + usr.sbin/smtpd/mta_session.c
--- usr.sbin/smtpd/mta_session.c
+++ usr.sbin/smtpd/mta_session.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -1604,6 +1605,10 @@ mta_cert_init_cb(void *arg, int status, const char *na
struct mta_session *s = arg;
void *ssl;
char *xname = NULL, *xcert = NULL;
+   union {
+   struct in_addr in4;
+   struct in6_addr in6;
+   } addrbuf;
 
if (s->flags & MTA_WAIT)
mta_tree_pop(_tls_init, s->id);
@@ -1623,6 +1628,22 @@ mta_cert_init_cb(void *arg, int status, const char *na
free(xcert);
if (ssl == NULL)
fatal("mta: ssl_mta_init");
+
+   /*
+* RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
+* permitted in "HostName".
+*/
+   if (s->relay->domain->as_host == 1) {
+   if (inet_pton(AF_INET, s->relay->domain->name, ) != 1 &&
+   inet_pton(AF_INET6, s->relay->domain->name, ) != 1) 
{
+   log_debug("%016"PRIx64" mta tls setting SNI name=%s",
+   s->id, s->relay->domain->name);
+   if (SSL_set_tlsext_host_name(ssl, 
s->relay->domain->name) == 0)
+   log_warnx("%016"PRIx64" mta tls setting SNI 
failed",
+  s->id);
+   }
+   }
+
io_start_tls(s->io, ssl);
 }
 



smtpd: make smarthost to use SNI when relaying

2020-05-30 Thread Sebastien Marie
Hi,

I am looking to make smtpd to set SNI (SSL_set_tlsext_host_name) when connecting
to smarthost when relaying mail.

After digging a bit in libtls (to stole the right code) and smtpd (to see where
to put the stolen code), I have the following diff:


diff 73b535ef4537e8454483912fc3420bc304759e96 /home/semarie/repos/openbsd/src
blob - d384692a0e43de47d645142a6b99e72b7d83b687
file + usr.sbin/smtpd/mta_session.c
--- usr.sbin/smtpd/mta_session.c
+++ usr.sbin/smtpd/mta_session.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -1604,6 +1605,8 @@ mta_cert_init_cb(void *arg, int status, const char *na
struct mta_session *s = arg;
void *ssl;
char *xname = NULL, *xcert = NULL;
+   struct in_addr addrbuf4;
+   struct in6_addr addrbuf6;
 
if (s->flags & MTA_WAIT)
mta_tree_pop(_tls_init, s->id);
@@ -1623,6 +1626,24 @@ mta_cert_init_cb(void *arg, int status, const char *na
free(xcert);
if (ssl == NULL)
fatal("mta: ssl_mta_init");
+
+   /*
+* RFC4366 (SNI): Literal IPv4 and IPv6 addresses are not
+* permitted in "HostName".
+*/
+   if (s->relay->domain->as_host == 1) {
+   xname = xstrdup(s->relay->domain->name);
+   if (inet_pton(AF_INET, xname, ) != 1 &&
+   inet_pton(AF_INET6, xname, ) != 1) {
+   log_info("%016"PRIx64" mta setting SNI name=%s",
+   s->id, xname);
+   if (SSL_set_tlsext_host_name(ssl, xname) == 0)
+   log_warnx("%016"PRIx64" mta setting SNI failed",
+  s->id);
+   }
+   free(xname);
+   }
+
io_start_tls(s->io, ssl);
 }
 


For what I understood:

mta_cert_init_cb() function is responsable to prepare a connection. the SSL
initialization (SSL_new() call) occured in ssl_mta_init() which was just called,
so it seems it is the right place to call SSL_set_tlsext_host_name().

We just need the hostname to configure it.

Regarding mta_session structure, relay->domain->as_host is set to 1 when the
domain is linked to smarthost configuration (or when the mx is ip address I
think). And in smarthost case, the domain->name is the hostname. For SNI, we are
excluding ip, so I assume it should copte with domain->name as ip.

Does someone with better understanding of smtpd code source could confirm the
approch is right and comment ?

Please note I have only tested it on simple configuration.

Thanks.
-- 
Sebastien Marie



Re: random(4): use arc4random_ctx_buf() for large device reads

2020-05-25 Thread Sebastien Marie
On Mon, May 25, 2020 at 05:27:37PM +0200, Christian Weisgerber wrote:
> Sebastien Marie:
> 
> > > For large reads from /dev/random, use the arc4random_ctx_*() functions
> > > instead of hand-rolling the same code to set up a temporary ChaCha
> > > instance.
> > 
> > Eventually, I would get ride of myctx, initialize lctx to NULL, and use
> > (lctx == NULL) to replace (myctx == 0).
> 
> Right.  Here we go:

Thanks. ok semarie@
 
> Index: rnd.c
> ===
> RCS file: /cvs/src/sys/dev/rnd.c,v
> retrieving revision 1.213
> diff -u -p -r1.213 rnd.c
> --- rnd.c 18 May 2020 15:00:16 -  1.213
> +++ rnd.c 25 May 2020 14:58:43 -
> @@ -589,6 +589,9 @@ arc4random_ctx_free(struct arc4random_ct
>  void
>  arc4random_ctx_buf(struct arc4random_ctx *ctx, void *buf, size_t n)
>  {
> +#ifndef KEYSTREAM_ONLY
> + memset(buf, 0, n);
> +#endif
>   chacha_encrypt_bytes((chacha_ctx *)ctx, buf, buf, n);
>  }
>  
> @@ -701,40 +704,31 @@ randomclose(dev_t dev, int flag, int mod
>  int
>  randomread(dev_t dev, struct uio *uio, int ioflag)
>  {
> - u_char  lbuf[KEYSZ+IVSZ];
> - chacha_ctx  lctx;
> + struct arc4random_ctx *lctx = NULL;
>   size_t  total = uio->uio_resid;
>   u_char  *buf;
> - int myctx = 0, ret = 0;
> + int ret = 0;
>  
>   if (uio->uio_resid == 0)
>   return 0;
>  
>   buf = malloc(POOLBYTES, M_TEMP, M_WAITOK);
> - if (total > RND_MAIN_MAX_BYTES) {
> - arc4random_buf(lbuf, sizeof(lbuf));
> - chacha_keysetup(, lbuf, KEYSZ * 8);
> - chacha_ivsetup(, lbuf + KEYSZ, NULL);
> - explicit_bzero(lbuf, sizeof(lbuf));
> - myctx = 1;
> - }
> + if (total > RND_MAIN_MAX_BYTES)
> + lctx = arc4random_ctx_new();
>  
>   while (ret == 0 && uio->uio_resid > 0) {
>   size_t  n = ulmin(POOLBYTES, uio->uio_resid);
>  
> - if (myctx) {
> -#ifndef KEYSTREAM_ONLY
> - memset(buf, 0, n);
> -#endif
> - chacha_encrypt_bytes(, buf, buf, n);
> - } else
> + if (lctx != NULL)
> + arc4random_ctx_buf(lctx, buf, n);
> + else
>   arc4random_buf(buf, n);
>   ret = uiomove(buf, n, uio);
>   if (ret == 0 && uio->uio_resid > 0)
>   yield();
>   }
> - if (myctx)
> - explicit_bzero(, sizeof(lctx));
> + if (lctx != NULL)
> + arc4random_ctx_free(lctx);
>   explicit_bzero(buf, POOLBYTES);
>   free(buf, M_TEMP, POOLBYTES);
>   return ret;
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de

-- 
Sebastien Marie



Re: random(4): use arc4random_ctx_buf() for large device reads

2020-05-25 Thread Sebastien Marie
On Sun, May 24, 2020 at 09:33:27PM +0200, Christian Weisgerber wrote:
> (This is in a different part of the file from Theo's current efforts.)
> 
> For large reads from /dev/random, use the arc4random_ctx_*() functions
> instead of hand-rolling the same code to set up a temporary ChaCha
> instance.
> 
> ok?

ok semarie@

Eventually, I would get ride of myctx, initialize lctx to NULL, and use
(lctx == NULL) to replace (myctx == 0). But feel free to discard my remark.

> 
> Index: rnd.c
> ===
> RCS file: /cvs/src/sys/dev/rnd.c,v
> retrieving revision 1.213
> diff -u -p -r1.213 rnd.c
> --- rnd.c 18 May 2020 15:00:16 -  1.213
> +++ rnd.c 24 May 2020 15:54:00 -
> @@ -589,6 +589,9 @@ arc4random_ctx_free(struct arc4random_ct
>  void
>  arc4random_ctx_buf(struct arc4random_ctx *ctx, void *buf, size_t n)
>  {
> +#ifndef KEYSTREAM_ONLY
> + memset(buf, 0, n);
> +#endif
>   chacha_encrypt_bytes((chacha_ctx *)ctx, buf, buf, n);
>  }
>  
> @@ -701,8 +704,7 @@ randomclose(dev_t dev, int flag, int mod
>  int
>  randomread(dev_t dev, struct uio *uio, int ioflag)
>  {
> - u_char  lbuf[KEYSZ+IVSZ];
> - chacha_ctx  lctx;
> + struct arc4random_ctx *lctx;
>   size_t  total = uio->uio_resid;
>   u_char  *buf;
>   int myctx = 0, ret = 0;
> @@ -712,29 +714,23 @@ randomread(dev_t dev, struct uio *uio, i
>  
>   buf = malloc(POOLBYTES, M_TEMP, M_WAITOK);
>   if (total > RND_MAIN_MAX_BYTES) {
> - arc4random_buf(lbuf, sizeof(lbuf));
> - chacha_keysetup(, lbuf, KEYSZ * 8);
> - chacha_ivsetup(, lbuf + KEYSZ, NULL);
> - explicit_bzero(lbuf, sizeof(lbuf));
> + lctx = arc4random_ctx_new();
>   myctx = 1;
>   }
>  
>   while (ret == 0 && uio->uio_resid > 0) {
>   size_t  n = ulmin(POOLBYTES, uio->uio_resid);
>  
> - if (myctx) {
> -#ifndef KEYSTREAM_ONLY
> - memset(buf, 0, n);
> -#endif
> - chacha_encrypt_bytes(, buf, buf, n);
> - } else
> + if (myctx)
> + arc4random_ctx_buf(lctx, buf, n);
> + else
>   arc4random_buf(buf, n);
>   ret = uiomove(buf, n, uio);
>   if (ret == 0 && uio->uio_resid > 0)
>   yield();
>   }
>   if (myctx)
> - explicit_bzero(, sizeof(lctx));
> + arc4random_ctx_free(lctx);
>   explicit_bzero(buf, POOLBYTES);
>   free(buf, M_TEMP, POOLBYTES);
>   return ret;
> -- 
> Christian "naddy" Weisgerber  na...@mips.inka.de
> 

-- 
Sebastien Marie



Re: pledge(2) sndioctl(1)

2020-05-22 Thread Sebastien Marie
On Fri, May 22, 2020 at 06:57:00AM +0200, Sebastien Marie wrote:
> On Thu, May 21, 2020 at 11:07:39PM +0100, Ricardo Mestre wrote:
> > Hi,
> > 
> > After the handle sioctl_hdl `hdl' is opened (which in itself requires rw fs
> > access and opening an unix socket) then all operations happen over that 
> > handle
> > so the program may be restricted to only "stdio".
> > 
> > All options were tested successfully, including the examples from the 
> > manpage
> > plus tweaking the audio from an app ($MYBROWSER).
> 
> Did you only perform runtime checking ? or also auditing the code source ?
> 
> Because depending your hardware and the way sndiod is configured, it isn't
> necessary the same code path used.
> 
> Just one example: with device "snd/0", the sioctl_open() will use
> _sioctl_aucat_open() for handler initialisation, whereas with "rsnd/0", it is
> _sioctl_sun_open().
> 
> As all functions called after sioctl_open() are using the handler, you might
> have tested only a part of the code path.
> 
> I didn't look deeper. So it might fine, but it could also break some
> configurations.
 
I looked a bit deeper.

commit()
 sioctl_setval()
   [with "rsnd/0"]
   sioctl_sun_setctl()
 setvol()
   ioctl(hdl->fd, AUDIO_MIXER_WRITE, )

calling ioctl(AUDIO_MIXER_WRITE) should required "audio".

whereas when using default "snd/0" device, it is calling sioctl_aucat_setctl()
and it will write(2) a message to sndiod(8) (and "stdio" is enough).

I agree that "rsnd/0" isn't the standard case (it requires root to read/write to
/dev), but it means you will broke sndioctl(1) in such cases.

Thanks.
-- 
Sebastien Marie



Re: pledge(2) sndioctl(1)

2020-05-21 Thread Sebastien Marie
On Thu, May 21, 2020 at 11:07:39PM +0100, Ricardo Mestre wrote:
> Hi,
> 
> After the handle sioctl_hdl `hdl' is opened (which in itself requires rw fs
> access and opening an unix socket) then all operations happen over that handle
> so the program may be restricted to only "stdio".
> 
> All options were tested successfully, including the examples from the manpage
> plus tweaking the audio from an app ($MYBROWSER).

Did you only perform runtime checking ? or also auditing the code source ?

Because depending your hardware and the way sndiod is configured, it isn't
necessary the same code path used.

Just one example: with device "snd/0", the sioctl_open() will use
_sioctl_aucat_open() for handler initialisation, whereas with "rsnd/0", it is
_sioctl_sun_open().

As all functions called after sioctl_open() are using the handler, you might
have tested only a part of the code path.

I didn't look deeper. So it might fine, but it could also break some
configurations.

Thanks.
-- 
Sebastien Marie



patch: usbdevs(8) cleanup

2019-12-21 Thread Sebastien Marie
Hi,

While looking at usbdevs(8) in order to see if it is possible to extended it a
bit to match specific devices (by driver, vendor, product, ...), I found it
isn't really simple to read at first glance.

This diff do mostly code cleanup and rearranging, but it include two real
changes from current code:

- a printf() switched to warn() in error code path for ioctl(2) (so stdout to
  stderr change, and it shows the real error instead of just "I/O error" for all
  error except ENXIO)

- USB_MAX_DEVICES is no more allowed in -a range, but it doesn't matter too much
  as ioctl(2) rejected it anyway with EINVAL.

Outside these two changes, others changes should be only cosmetic or readability
changes. No others functional changes intented.


Globally:
- rename usbdev() to dump_device()
- rename dumpone() to dump_controller()
- merge usbdump() to dump_controller(): usbdump() was just about iterating on
  all possible address devices and calling usbdev() [dump_device] on each
- group the two global variables at top level: move `done' array next to 
`verbose`
- collapse syscall + if condition in one-line
  f = open(); if (f >= 0) { ... } else { ... })

  becomes the usual:
  if ((f = open()) < 0) { ... }
  
- unify addr type to uint8_t instead of having it uint8_t in some places and 
int in others


In the main function:
- use more explicit names for variables
  - buf -> path : for the controller path, and use PATH_MAX size (50 is 
technically fine as it is build from "/dev/usb%d")
  alternatively commenting the size could be fine too
  - dev -> controller : code is about usb devices and usb controllers. here it 
is a controller path (/dev/usbN)
  - f -> fd : use variable name a bit more common for a file-descriptor
- move some variables declaration to deeper block

dump_device function (ex usbdev):
- check if(var[0] == '\0') instead of if(strlen(var))
- uniform use of printf(", element") instead of having some printf("element, ") 
and some printf(", element") and other printf("element")
- move some variables declaration to deeper block


I could do incremental changes too.

Comments or OK ?
-- 
Sebastien Marie


diff da270b4d7186cb04c0a40c70d7f708d48fe543a9 /home/semarie/repos/openbsd/src
blob - 165f668b527d6e794e57a62842e313fbf620e22e
file + usr.sbin/usbdevs/usbdevs.c
--- usr.sbin/usbdevs/usbdevs.c
+++ usr.sbin/usbdevs/usbdevs.c
@@ -52,11 +52,11 @@
 #define USBDEV "/dev/usb"
 
 int verbose = 0;
+char done[USB_MAX_DEVICES];
 
 void usage(void);
-void usbdev(int f, uint8_t);
-void usbdump(int f);
-void dumpone(char *name, int f, int addr);
+void dump_device(int, uint8_t);
+void dump_controller(char *, int, uint8_t);
 int main(int, char **);
 
 extern char *__progname;
@@ -68,79 +68,83 @@ usage(void)
exit(1);
 }
 
-char done[USB_MAX_DEVICES];
-
 void
-usbdev(int f, uint8_t addr)
+dump_device(int fd, uint8_t addr)
 {
struct usb_device_info di;
-   int e, i, port, nports;
-   uint16_t change, status;
+   int i;
char vv[sizeof(di.udi_vendor)*4], vp[sizeof(di.udi_product)*4];
char vr[sizeof(di.udi_release)*4], vs[sizeof(di.udi_serial)*4];
 
di.udi_addr = addr;
-   e = ioctl(f, USB_DEVICEINFO, );
-   if (e) {
+   if (ioctl(fd, USB_DEVICEINFO, ) == -1) {
if (errno != ENXIO)
-   printf("addr %d: I/O error\n", addr);
+   warn("addr %u", addr);
return;
}
 
-   printf("addr %02u: ", addr);
done[addr] = 1;
+
strvis(vv, di.udi_vendor, VIS_CSTYLE);
strvis(vp, di.udi_product, VIS_CSTYLE);
-   printf("%04x:%04x %s, %s", di.udi_vendorNo, di.udi_productNo,
+   printf("addr %02u: %04x:%04x %s, %s", addr,
+   di.udi_vendorNo, di.udi_productNo,
vv, vp);
 
if (verbose) {
printf("\n\t ");
switch (di.udi_speed) {
case USB_SPEED_LOW:
-   printf("low speed, ");
+   printf("low speed");
break;
case USB_SPEED_FULL:
-   printf("full speed, ");
+   printf("full speed");
break;
case USB_SPEED_HIGH:
-   printf("high speed, ");
+   printf("high speed");
break;
case USB_SPEED_SUPER:
-   printf("super speed, ");
+   printf("super speed");
break;
default:
break;
}
 
if (di.udi_power)
-   printf("power %d mA, ", di.udi_power);
+   pri

Re: snmp(1): Add set command

2019-10-03 Thread Sebastien Marie
On Thu, Oct 03, 2019 at 10:55:51AM +0200, Martijn van Duren wrote:
> > 
> > But even if the agent is wrong, I think it is dangerous to bindly trust the 
> > other
> > side of accessing a memory chunk. Before using the value, it should be 
> > checked
> > against the valid range to avoid uncontrolled memory access.
> > 
> > For me, for such ill response, snmp should exit with error.
> > 
> > FYI, snmpset does the following:
> > 
> > $ snmpset -v2c -c public 192.168.92.5 sysDescr.0 = "test"
> > Error in packet.
> > Reason: notWritable (That object does not support modification)
> > 
> > Thanks.
> > 
> I agree, but considering this is also part of all the other command, OK
> to commit this as is and fix it for all the other in one sweep?

hep, it doesn't add more problems that what is already in. ok semarie@ as it.

-- 
Sebastien Marie



Re: snmp(1): Add set command

2019-10-03 Thread Sebastien Marie
On Thu, Oct 03, 2019 at 10:01:06AM +0200, Martijn van Duren wrote:
> >> Index: snmpc.c
> >> ===
> >> RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
> >> retrieving revision 1.11
> >> diff -u -p -r1.11 snmpc.c
> >> --- snmpc.c18 Sep 2019 09:54:36 -  1.11
> >> +++ snmpc.c26 Sep 2019 12:32:38 -
> >> @@ -666,19 +669,54 @@ snmpc_walk(int argc, char *argv[])
> >>  }
> >>  
> >>  int
> >> +snmpc_set(int argc, char *argv[])
> >> +{
> >> +  struct snmp_agent *agent;
> >> +  struct ber_element *pdu, *varbind;
> >> +  int errorstatus, errorindex;
> >> +  int class;
> >> +  unsigned type;
> >> +
> >> +  if (argc < 4)
> >> +  usage();
> >> +  if ((agent = snmpc_connect(argv[0], "161")) == NULL)
> >> +  err(1, "%s", snmp_app->name);
> >> +  argc--;
> >> +  argv++;
> >> +
> >> +  if (argc < 3 || argc % 3 != 0)
> >> +  usage();
> >> +
> >> +  if (pledge("stdio", NULL) == -1)
> >> +  err(1, "pledge");
> >> +
> >> +  pdu = snmp_set(agent, snmpc_varbindparse(argc, argv));
> >> +
> >> +  (void) ber_scanf_elements(pdu, "t{Sdd{e", , , ,
> >> +  , );
> >> +  if (errorstatus != 0)
> >> +  snmpc_printerror((enum snmp_error) errorstatus,
> >> +  argv[errorindex - 1]);
> > 
> > is "errorindex - 1" the right index ?
> 
> Yes: RFC 3416 section 4.1:
> A variable binding is identified by its index value.  The first variable
> binding in a variable-binding list is index one, the second is index 
> two, etc.
> 
> I choose to use argv, because it's easier for the end user to see his
> own input for where he mistyped instead of a double parsed oid to -O
> based output.
> 
> > $ ./obj/snmp set 192.168.1.5 sysContact.0 s "test"
> > snmp: Can't parse oid 192.168.1.5: Not writable
> > 
> > Note that the same pattern is used in others commands.
> 
> Somehow the machines I test against don't return a not writable message.
> Tested against snmpd, net-snmpd, HP Laserjet.

The agent is on a TOSHIBA e-STUDIO3505AC device.

> Could you show me a tcpdump -v output on this output?

It returns 0 as index.

for the command: ./obj/snmp set 192.168.1.5 sysContact.0 s "test"

$ tcpdump -r test.pcap -n
10:06:12.953745 192.168.1.12.4864 > 192.168.1.5.161: SetRequest(32) 
.1.3.6.1.2.1.1.4.0="test"
10:06:12.954552 192.168.1.5.161 > 192.168.1.12.4864: GetResponse(32) 
notWritable[errorIndex==0] .1.3.6.1.2.1.1.4.0="test" (DF)

$ tcpdump -r test.pcap -nvvv
10:06:12.953745 192.168.1.12.4864 > 192.168.1.5.161: [udp sum ok] 
|30|2d|02|01SNMPv2c |04|06|a3|20SetRequest(32)|02|04 
reqId=233006489|02|01|02|01|30|12 |30|10|06|08.1.3.6.1.2.1.1.4.0=|04|04"test" 
(ttl 64, id 50468, len 75, bad ip cksum 0! -> 7c1b)
10:06:12.954552 192.168.1.5.161 > 192.168.1.12.4864: [udp sum ok] 
|30|2d|02|01SNMPv2c |04|06|a2|20GetResponse(32)|02|04 reqId=233006489|02|01 
notWritable|02|01[errorIndex==0]|30|12 
|30|10|06|08.1.3.6.1.2.1.1.4.0=|04|04"test" (DF) (ttl 64, id 20430, len 75)

And after more check, the index problem is without problem on toshiba side:
depending the community used, the returned oid (and index) is wrong.

$ snmp set -c private 192.168.1.5 sysDescr.0 s "test"
snmp: Can't parse oid sysDescr.0: Not writable

./obj/snmp set -c public 192.168.1.5 sysDescr.0 s "test"
snmp: Can't parse oid 192.168.1.5: Not writable

(I changed the oid because with 'private' I am able to write to sysContact.0)


But even if the agent is wrong, I think it is dangerous to bindly trust the 
other
side of accessing a memory chunk. Before using the value, it should be checked
against the valid range to avoid uncontrolled memory access.

For me, for such ill response, snmp should exit with error.

FYI, snmpset does the following:

$ snmpset -v2c -c public 192.168.92.5 sysDescr.0 = "test"
Error in packet.
Reason: notWritable (That object does not support modification)

Thanks.
-- 
Sebastien Marie



Re: snmp(1): Add set command

2019-10-03 Thread Sebastien Marie
  , str, strl)) == NULL)
> + err(1, "ber_printf_elements");
> + free(str);
> + break;
> + case 't':
> + lval = strtonum(argv[i + 2], LLONG_MIN, LLONG_MAX,
> + );
> + if (errstr != NULL)
> + errx(1, "%s: Bad value notation (%s)", argv[i],
> + argv[i + 2]);
> + if ((varbind = ber_printf_elements(varbind, "{Oit}",
> + , lval, BER_CLASS_APPLICATION,
> + SNMP_T_TIMETICKS)) == NULL)
> + err(1, "ber_printf_elements");
> + break;
> + case 'x':
> + /* String always shrinks */
> + if ((str = malloc(strlen(argv[i + 2]))) == NULL)
> + err(1, "malloc");
> + tmpstr = argv[i + 2];
> + strl = 0;
> + do {
> +     lval = strtoll(tmpstr, , 16);
> + if (endstr[0] != ' ' && endstr[0] != '\t' &&
> + endstr[0] != '\0')
> + errx(1, "%s: Bad value notation (%s)",
> + argv[i], argv[i + 2]);
> + if (tmpstr == endstr) {
> + tmpstr++;
> + continue;
> + }
> + if (lval < 0 || lval > 0xff)
> + errx(1, "%s: Bad value notation (%s)",
> + argv[i], argv[i + 2]);
> + str[strl++] = (unsigned char) lval;
> + tmpstr = endstr + 1;
> + } while (endstr[0] != '\0');
> + goto pastestring;
> + default:
> + usage();
> + }
> + if (vblist == NULL)
> + vblist = varbind;
> + }
> +
> + return vblist;
>  }
>  
>  __dead void
> 

-- 
Sebastien Marie



snmp: invalid error message

2019-10-03 Thread Sebastien Marie
Hi,

While testing snmp(1), I found the following weird behaviour regarding error
message:

$ snmp get 192.168.1.5 sysDescr.0 sysUpTime.0
sysDescr.0 = STRING: TOSHIBA e-STUDIO3505AC
sysUpTime.0 = Timeticks: (55587200) 6 day 10:24:32.00

$ snmp get 192.168.1.5 sysDescr.0 sysUpTime.0 xxx
snmp: sysDescr.0: Unknown object identifier

The first command returns valid values. For the second, where the third oid is
invalid, the error message speak about the first oid.

The following diff corrects the error message by picking the right argv element.

Comments or OK ?
-- 
Sebastien Marie

Index: snmpc.c
===
RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
retrieving revision 1.11
diff -u -p -r1.11 snmpc.c
--- snmpc.c 18 Sep 2019 09:54:36 -  1.11
+++ snmpc.c 3 Oct 2019 06:50:19 -
@@ -496,7 +496,7 @@ snmpc_get(int argc, char *argv[])
err(1, "malloc");
for (i = 0; i < argc; i++) {
if (smi_string2oid(argv[i], [i]) == -1)
-   errx(1, "%s: Unknown object identifier", argv[0]);
+   errx(1, "%s: Unknown object identifier", argv[i]);
}
if (strcmp(snmp_app->name, "getnext") == 0) {
if ((pdu = snmp_getnext(agent, oid, argc)) == NULL)



patch: libtool: copte with potentially undefined dependency_libs and libdir

2019-09-26 Thread Sebastien Marie
Hi,

Latest version of librsvg (2.46.0) uses libtool-rust, which produce .la file a
bit different from gnu libtool.  in particular, dependency_libs and libdir could
be undefined.

but us libtool version doesn't like that: when libdir is undefined it makes
warnings, and just die for dependency_libs (trying to use undef as an array
doesn't work).

The following patch makes us libtool to define these elements if there are
undefined after parsing the .la file.  With that, I was able to build
librsvg-2.46.0 whereas without it just fail.

Due to special nature of libtool, and current position in the release cycle, I
would like to have feedback for this diff with a bulk build, to be sure to not
breaking anything.

Please note I need to update librsvg in order to commit lang/rust 1.38.0 (the
current version of librsvg doesn't build with 1.38.0).

Comments or OK (if the bulk successed) ?
-- 
Sebastien Marie


Index: LT/LaFile.pm
===
RCS file: /cvs/src/usr.bin/libtool/LT/LaFile.pm,v
retrieving revision 1.23
diff -u -p -r1.23 LaFile.pm
--- LT/LaFile.pm5 Sep 2014 10:36:39 -   1.23
+++ LT/LaFile.pm26 Sep 2019 12:11:17 -
@@ -188,4 +188,18 @@ sub install
}
 }
 
+sub parse
+{
+   my ($class, $filename) = @_;
+
+   my $info = $class->SUPER::parse($filename);
+
+   $info->{deplib_list} = LT::UList->new
+   if (not defined $info->{deplib_list});
+   $info->{libdir} = ''
+   if (not defined $info->{libdir});
+
+   return $info;
+}
+
 1;



smtpd: parse.y copy/paste error

2019-09-21 Thread Sebastien Marie
Hi,

A copy/paste error is present in parse.y, due to late addition of "srs key 
backup secrets".

- grammar needs a | to make the following part of 'srs' section
- the backup secret is $4 (and not $3)

Comments or OK ?
-- 
Sebastien Marie

diff 3a973d3129d0b88d07e9b2c5cb9e9cfcaf7997d7 /home/semarie/repos/openbsd/src
blob - 4801d14f3f2e47b76df581bc63a99fa10572
file + usr.sbin/smtpd/parse.y
--- usr.sbin/smtpd/parse.y
+++ usr.sbin/smtpd/parse.y
@@ -542,8 +542,8 @@ srs:
 SRS KEY STRING {
conf->sc_srs_key = $3;
 }
-SRS KEY BACKUP STRING {
-   conf->sc_srs_key_backup = $3;
+| SRS KEY BACKUP STRING {
+   conf->sc_srs_key_backup = $4;
 }
 | SRS TTL STRING {
conf->sc_srs_ttl = delaytonum($3);



smtpd: smtpc: ssl_check_name() dead assignment

2019-09-21 Thread Sebastien Marie
Hi,

The current code in smtp_verify_server_cert() has a dead assignment for return
code of ssl_check_name().

At first stance, I was a bit unsure if return code should be checked or not to
detect certificate error. but as soon ssl_check_name() is entered, `match' is
set to 0, and only set to 1 when the name is know to be good.

so I assume it is fine to ignore the return code.

please note that ssl_check_name() is only called at twice places:
- smtpc.c, here
- mta_session.c, where return code isn't checked too

so maybe the function API should be changed ? or to return void, or to return
the match value ?

Thanks.
-- 
Sebastien Marie

diff ea5e035f4d57ede9f18c82c5c9decc5f46c1925a /home/semarie/repos/openbsd/src
blob - fb6d711d95f3a8203e44d6662002a32c92a89629
file + usr.sbin/smtpd/smtpc.c
--- usr.sbin/smtpd/smtpc.c
+++ usr.sbin/smtpd/smtpc.c
@@ -351,10 +351,10 @@ smtp_verify_server_cert(void *tag, struct smtp_client 
SSL *ssl = ctx;
X509 *cert;
long res;
-   int r, match;
+   int match;
 
if ((cert = SSL_get_peer_certificate(ssl))) {
-   r = ssl_check_name(cert, servname, );
+   (void)ssl_check_name(cert, servname, );
X509_free(cert);
res = SSL_get_verify_result(ssl);
if (res == X509_V_OK) {



smtpd: smtp_session.c: remove simple dead assignment

2019-09-21 Thread Sebastien Marie
Hi,

The following diff removes a simple i=0 dead assignment. There is no need to set
it before entering the for() loop which will set it anyway.

Comments or OK ?
-- 
Sebastien Marie


diff ea5e035f4d57ede9f18c82c5c9decc5f46c1925a /home/semarie/repos/openbsd/src
blob - 4e4978e48478815eb789b777ff4d8caf4e455b7b
file + usr.sbin/smtpd/smtp_session.c
--- usr.sbin/smtpd/smtp_session.c
+++ usr.sbin/smtpd/smtp_session.c
@@ -295,13 +295,12 @@ header_append_domain_buffer(char *buffer, char *domain
size_t  i;
int escape, quote, comment, bracket;
int has_domain, has_bracket, has_group;
int pos_bracket, pos_component, pos_insert;
charcopy[APPEND_DOMAIN_BUFFER_SIZE];
 
-   i = 0;
escape = quote = comment = bracket = 0;
has_domain = has_bracket = has_group = 0;
pos_bracket = pos_insert = pos_component = 0;
for (i = 0; buffer[i]; ++i) {
if (buffer[i] == '(' && !escape && !quote)
comment++;



smtpd: report_smtp_broadcast: error out on unexpected direction

2019-09-21 Thread Sebastien Marie
Hi,

in report_smtp_broadcast(), the direction is expected to be "smtp-in" or
"smtp-out", but if something else happen, the `struct dict *d' will be
uninitialized when used in `dict_xget(d, event)'.

so enforce the direction is right by make an unexpected direction fatal.

Please note that it makes a logic error in smtpd to be visible instead of
accessing uninitialized memory. I am not expecting direction to come from
external source.

Comments or OK ?
-- 
Sebastien Marie

diff ea5e035f4d57ede9f18c82c5c9decc5f46c1925a /home/semarie/repos/openbsd/src
blob - 8b745935036023a0d493ca839555b62d01cfce87
file + usr.sbin/smtpd/lka_report.c
--- usr.sbin/smtpd/lka_report.c
+++ usr.sbin/smtpd/lka_report.c
@@ -150,18 +150,21 @@ report_smtp_broadcast(uint64_t reqid, const char *dire
va_list ap;
struct dict *d;
struct reporters*tailq;
struct reporter_proc*rp;
 
if (strcmp("smtp-in", direction) == 0)
d = _in;
 
-   if (strcmp("smtp-out", direction) == 0)
+   else if (strcmp("smtp-out", direction) == 0)
d = _out;
+
+   else
+   fatalx("unexpected direction: %s", direction);
 
tailq = dict_xget(d, event);
TAILQ_FOREACH(rp, tailq, entries) {
if (!lka_filter_proc_in_session(reqid, rp->name))
continue;
 
va_start(ap, format);
if (io_printf(lka_proc_get_io(rp->name),



smtpd: ecdsa_engine_init: properly initialize errstr on error

2019-09-21 Thread Sebastien Marie
Hi,

in ecdsa_engine_init(), if ECDSA_METHOD_new_temporary() failed and return NULL,
the goto fail will use errstr which is uninitialized.

749  fail:
750 ssl_error(errstr);
751 fatalx("%s", errstr);

Set `errstr' to static string as all others error condition in the function.

Comments or OK ?

Thanks.
-- 
Sebastien Marie

diff ea5e035f4d57ede9f18c82c5c9decc5f46c1925a /home/semarie/repos/openbsd/src
blob - 7afcfb7d247e78677368cdf983e1fedb6bcda0b7
file + usr.sbin/smtpd/ca.c
--- usr.sbin/smtpd/ca.c
+++ usr.sbin/smtpd/ca.c
@@ -705,8 +705,10 @@ ecdsa_engine_init(void)
ENGINE  *e;
const char  *errstr, *name;
 
-   if ((ecdsae_method = ECDSA_METHOD_new_temporary("ECDSA privsep engine", 
0)) == NULL)
+   if ((ecdsae_method = ECDSA_METHOD_new_temporary("ECDSA privsep engine", 
0)) == NULL) {
+   errstr = "ECDSA_METHOD_new_temporary";
goto fail;
+   }
 
ecdsae_method->ecdsa_do_sign = ecdsae_do_sign;
ecdsae_method->ecdsa_sign_setup = ecdsae_sign_setup;



Re: Make filter line handling more developer friendly

2019-08-26 Thread Sebastien Marie
On Sun, Aug 25, 2019 at 07:01:01AM +0200, Martijn van Duren wrote:
> Right now all we get is "Misbehaving filter", which doesn't tell the
> developer a lot.
> 
> Diff below does the following:
> - Make the register_hooks actually fail on misbehaviour.
> - Change some *ep = 0 to a more semantically correct ep[0] = '\0'
> - Hoist some checks from lka_filter_process_response to processor_io
> - Make lka_filter_process_response void (it fatals now)
> - strtoull returns ULLONG_MAX on error, not ULONG_MAX
> - change str{,n}casecmp to str{,n}cmp for consistency.
> - change an fugly "nextline:" "goto nextline" loop to an actual while.
> - restructure lka_filter_process_response to be shorter.
> and most importantly
> - Add a descriptive fatal() minefield.
> 
> -12 LoC.
> 
> Tested with filter-dnsbl with both a successful and rejected connection.
> 
> OK?

just one remark.

> Index: lka_filter.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 lka_filter.c
> --- lka_filter.c  18 Aug 2019 16:52:02 -  1.41
> +++ lka_filter.c  25 Aug 2019 04:47:47 -
> @@ -429,88 +429,68 @@ lka_filter_process_response(const char *
>   struct filter_session *fs;
>  
>   (void)strlcpy(buffer, line, sizeof buffer);
> - if ((ep = strchr(buffer, '|')) == NULL)
> - return 0;
> - *ep = 0;
> +     ep = strchr(buffer, '|');
> + ep[0] = '\0';

is it possible to buffer to not have '|' ? if yes, you could deference NULL.
  
-- 
Sebastien Marie



Re: smtpd: Allow labels containing "@"

2019-07-23 Thread Sebastien Marie
On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> My mail is klem...@posteo.de and the provider expects this full address
> as username, so that makes for the following perfectly
> valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.

it seems to me this url is wrong. the '@' in username should be urlencoded.

smtps://klemens%40posteo...@posteo.de:465.

RFC3986 Uniform Resource Identifier (URI): Generic Syntax

  3.2.1.  User Information

The userinfo subcomponent may consist of a user name and, optionally,
scheme-specific information about how to gain authorization to access
the resource.  The user information, if present, is followed by a
commercial at-sign ("@") that delimits it from the host.

userinfo= *( unreserved / pct-encoded / sub-delims / ":" )


> I've been doing that with mutt(1) for ages.
> 
> smtpd.conf(5) has the following syntax:
> 
>   host relay-url
>   Do not perform MX lookups but relay messages to the relay
>   host described by relay-url.  The format for relay-url is
>   [proto://[label@]]host[:port].  The following protocols
>   are available:
> 
> yielding the following config in my case:
> 
>   action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth 
> 
> 
> However, when parsing the label in the `relay-url', smtpd(8) stops at
> the first "@" sign, not expecting labels to contain it.  The following
> diff fixes this spanning the label to the last occurence of "@" as is
> already done in other code places.
> 
> Feedback? Objections?
> OK?
> 
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 to.c
> --- to.c  30 Dec 2018 23:09:58 -  1.35
> +++ to.c  22 Jul 2019 21:02:48 -
> @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
>   relay->port = 0;
>  
>   /* first, we extract the label if any */
> - if ((q = strchr(p, '@')) != NULL) {
> + if ((q = strrchr(p, '@')) != NULL) {
>   *q = 0;
>   if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
>   >= sizeof (relay->authlabel))
> 

-- 
Sebastien Marie



Re: pipe: revisiting pipe: step 2: pipe_create() / pipe_free()

2019-07-15 Thread Sebastien Marie
On Mon, Jul 15, 2019 at 12:14:00PM +0200, Sebastien Marie wrote:
> Hi,
> 
> Next move in revisiting pipe initialization.
> 
> After some discussion with mpi@, it seems better to have the whole
> `struct pipe' allocation and initialization inside pipe_create()
> function, aka moving pool_get() inside pipe_create() instead of
> allocating the struct in dopipe() and initialize it in pipe_create().
> 
> It makes pipe_create() to return a `struct pipe *' instead of an error
> code. The sole possible error was ENOMEM, so returning NULL in such case
> is fine.
> 
> 
> In the same move, I revisited the freeing of the struct pipe: the
> pipeclose() function.
> 
> About pipeclose():
> 
> - I dislike having both pipe_close() and pipeclose() functions. So I
>   renamed pipeclose() to pipe_free(). The function does a bit more that
>   freeing resources, as it ensures all customers possibling in wait state
>   are aware of the pipe terminaison. but I don't have better name...
> 
> - I inversed the if-condition to return early, and move the whole
>   if-body in the function. It is more readable, and match what we usually
>   do in such case.
> 
> This way, pipe_create() and pipe_free() are symetric: allocation in one,
> and free in another.

new diff with changes asked by claudio@. I also used pipe_destroy()
name.

I send a new diff as a little omission was present in pipe_create(): it
still returned 0 on success (instead of the newly allocated pointer).
bad semarie@ that didn't test its own diff after splitting it to make a
small diff.

-- 
Sebastien Marie


Index: kern/sys_pipe.c
===
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.94
diff -u -p -r1.94 sys_pipe.c
--- kern/sys_pipe.c 15 Jul 2019 09:05:46 -  1.94
+++ kern/sys_pipe.c 15 Jul 2019 16:08:42 -
@@ -97,12 +97,12 @@ static unsigned int amountpipekva;
 struct pool pipe_pool;
 
 intdopipe(struct proc *, int *, int);
-void   pipeclose(struct pipe *);
-intpipe_create(struct pipe *);
 intpipelock(struct pipe *);
 void   pipeunlock(struct pipe *);
 void   pipeselwakeup(struct pipe *);
 
+struct pipe *pipe_create(void);
+void   pipe_destroy(struct pipe *);
 intpipe_buffer_realloc(struct pipe *, u_int);
 void   pipe_buffer_free(struct pipe *);
 
@@ -144,14 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
 
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-   rpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
-   error = pipe_create(rpipe);
-   if (error != 0)
-   goto free1;
-   wpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
-   error = pipe_create(wpipe);
-   if (error != 0)
+   if (((rpipe = pipe_create()) == NULL) ||
+   ((wpipe = pipe_create()) == NULL)) {
+   error = ENOMEM;
goto free1;
+   }
 
fdplock(fdp);
 
@@ -202,8 +199,8 @@ free3:
 free2:
fdpunlock(fdp);
 free1:
-   pipeclose(wpipe);
-   pipeclose(rpipe);
+   pipe_destroy(wpipe);
+   pipe_destroy(rpipe);
return (error);
 }
 
@@ -247,22 +244,27 @@ pipe_buffer_realloc(struct pipe *cpipe, 
 /*
  * initialize and allocate VM and memory for pipe
  */
-int
-pipe_create(struct pipe *cpipe)
+struct pipe *
+pipe_create(void)
 {
+   struct pipe *cpipe;
int error;
 
-   sigio_init(>pipe_sigio);
+   cpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
 
error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
-   if (error != 0)
-   return (error);
+   if (error != 0) {
+   pool_put(_pool, cpipe);
+   return (NULL);
+   }
+
+   sigio_init(>pipe_sigio);
 
getnanotime(>pipe_ctime);
cpipe->pipe_atime = cpipe->pipe_ctime;
cpipe->pipe_mtime = cpipe->pipe_ctime;
 
-   return (0);
+   return (cpipe);
 }
 
 
@@ -768,7 +770,7 @@ pipe_close(struct file *fp, struct proc 
fp->f_ops = NULL;
fp->f_data = NULL;
KERNEL_LOCK();
-   pipeclose(cpipe);
+   pipe_destroy(cpipe);
KERNEL_UNLOCK();
return (0);
 }
@@ -799,44 +801,46 @@ pipe_buffer_free(struct pipe *cpipe)
 }
 
 /*
- * shutdown the pipe
+ * shutdown the pipe, and free resources.
  */
 void
-pipeclose(struct pipe *cpipe)
+pipe_destroy(struct pipe *cpipe)
 {
struct pipe *ppipe;
-   if (cpipe) {
-   pipeselwakeup(cpipe);
-   sigio_free(>pipe_sigio);
 
-   /*
-* If the other side is blocked, wake it up saying that
-* we want to close it down.
-*/
-   cpipe->pipe_state |= PIPE_EOF;
-   while (cpipe->pipe_busy) {
-   wakeup(cpipe);
-   cpipe->pipe_state |= PIPE_WANTD;
-   tsleep(cpipe, PRIBIO, "

pipe: revisiting pipe: step 2: pipe_create() / pipe_free()

2019-07-15 Thread Sebastien Marie
Hi,

Next move in revisiting pipe initialization.

After some discussion with mpi@, it seems better to have the whole
`struct pipe' allocation and initialization inside pipe_create()
function, aka moving pool_get() inside pipe_create() instead of
allocating the struct in dopipe() and initialize it in pipe_create().

It makes pipe_create() to return a `struct pipe *' instead of an error
code. The sole possible error was ENOMEM, so returning NULL in such case
is fine.


In the same move, I revisited the freeing of the struct pipe: the
pipeclose() function.

About pipeclose():

- I dislike having both pipe_close() and pipeclose() functions. So I
  renamed pipeclose() to pipe_free(). The function does a bit more that
  freeing resources, as it ensures all customers possibling in wait state
  are aware of the pipe terminaison. but I don't have better name...

- I inversed the if-condition to return early, and move the whole
  if-body in the function. It is more readable, and match what we usually
  do in such case.

This way, pipe_create() and pipe_free() are symetric: allocation in one,
and free in another.

Comments or OK ?
-- 
Sebastien Marie


Index: sys/kern/sys_pipe.c
===
--- sys/kern/sys_pipe.c.orig2019-07-15 11:06:05.015708914 +0200
+++ sys/kern/sys_pipe.c 2019-07-15 11:11:35.667317769 +0200
@@ -97,12 +97,12 @@ static unsigned int amountpipekva;
 struct pool pipe_pool;
 
 intdopipe(struct proc *, int *, int);
-void   pipeclose(struct pipe *);
-intpipe_create(struct pipe *);
 intpipelock(struct pipe *);
 void   pipeunlock(struct pipe *);
 void   pipeselwakeup(struct pipe *);
 
+struct pipe *pipe_create();
+void   pipe_free(struct pipe *);
 intpipe_buffer_realloc(struct pipe *, u_int);
 void   pipe_buffer_free(struct pipe *);
 
@@ -144,14 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
 
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-   rpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
-   error = pipe_create(rpipe);
-   if (error != 0)
-   goto free1;
-   wpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
-   error = pipe_create(wpipe);
-   if (error != 0)
+   if (((rpipe = pipe_create()) == NULL) ||
+   ((wpipe = pipe_create()) == NULL)) {
+   error = ENOMEM;
goto free1;
+   }
 
fdplock(fdp);
 
@@ -202,8 +199,8 @@ free3:
 free2:
fdpunlock(fdp);
 free1:
-   pipeclose(wpipe);
-   pipeclose(rpipe);
+   pipe_free(wpipe);
+   pipe_free(rpipe);
return (error);
 }
 
@@ -247,16 +244,19 @@ pipe_buffer_realloc(struct pipe *cpipe,
 /*
  * initialize and allocate VM and memory for pipe
  */
-int
-pipe_create(struct pipe *cpipe)
+struct pipe *
+pipe_create()
 {
int error;
-
-   sigio_init(>pipe_sigio);
+   struct pipe *cpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
 
error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
-   if (error != 0)
-   return (error);
+   if (error != 0) {
+   pool_put(_pool, cpipe);
+   return (NULL);
+   }
+
+   sigio_init(>pipe_sigio);
 
getnanotime(>pipe_ctime);
cpipe->pipe_atime = cpipe->pipe_ctime;
@@ -768,7 +768,7 @@ pipe_close(struct file *fp, struct proc
fp->f_ops = NULL;
fp->f_data = NULL;
KERNEL_LOCK();
-   pipeclose(cpipe);
+   pipe_free(cpipe);
KERNEL_UNLOCK();
return (0);
 }
@@ -799,44 +799,46 @@ pipe_buffer_free(struct pipe *cpipe)
 }
 
 /*
- * shutdown the pipe
+ * shutdown the pipe, and free resources.
  */
 void
-pipeclose(struct pipe *cpipe)
+pipe_free(struct pipe *cpipe)
 {
struct pipe *ppipe;
-   if (cpipe) {
-   pipeselwakeup(cpipe);
-   sigio_free(>pipe_sigio);
 
-   /*
-* If the other side is blocked, wake it up saying that
-* we want to close it down.
-*/
-   cpipe->pipe_state |= PIPE_EOF;
-   while (cpipe->pipe_busy) {
-   wakeup(cpipe);
-   cpipe->pipe_state |= PIPE_WANTD;
-   tsleep(cpipe, PRIBIO, "pipecl", 0);
-   }
+   if (cpipe == NULL)
+   return;
 
-   /*
-* Disconnect from peer
-*/
-   if ((ppipe = cpipe->pipe_peer) != NULL) {
-   pipeselwakeup(ppipe);
+   pipeselwakeup(cpipe);
+   sigio_free(>pipe_sigio);
 
-   ppipe->pipe_state |= PIPE_EOF;
-   wakeup(ppipe);
-   ppipe->pipe_peer = NULL;
-   }
+   /*
+* If the other side is blocked, wake it up saying that
+* we want to close it down.
+*/
+   cpipe->pipe_state |= PIPE_EOF;
+   while (cpipe->pipe_busy) {

pipe: rename PIPE_WANT flag

2019-07-13 Thread Sebastien Marie
Hi,

The following diff renames PIPE_WANT flag to PIPE_WANTD.

PIPE_WANT flag is used for signaling the pipe is about to be run-down.
Pending readers/writers will wakeup the closing thread which is waiting.

We already have PIPE_WANTR, PIPE_WANTW and PIPE_LWANT flags, so
PIPE_WANT isn't really descriptive.

No functional changes intented.

Comments or OK ?
-- 
Sebastien Marie


Index: sys/kern/sys_pipe.c
===
--- sys/kern/sys_pipe.c.orig2019-07-13 07:06:05.239550881 +0200
+++ sys/kern/sys_pipe.c 2019-07-13 07:07:48.780729506 +0200
@@ -399,10 +399,10 @@ unlocked_error:
--rpipe->pipe_busy;
 
/*
-* PIPE_WANT processing only makes sense if pipe_busy is 0.
+* PIPE_WANTD processing only makes sense if pipe_busy is 0.
 */
-   if ((rpipe->pipe_busy == 0) && (rpipe->pipe_state & PIPE_WANT)) {
-   rpipe->pipe_state &= ~(PIPE_WANT|PIPE_WANTW);
+   if ((rpipe->pipe_busy == 0) && (rpipe->pipe_state & PIPE_WANTD)) {
+   rpipe->pipe_state &= ~(PIPE_WANTD|PIPE_WANTW);
wakeup(rpipe);
} else if (rpipe->pipe_buffer.cnt < MINPIPESIZE) {
/*
@@ -470,8 +470,8 @@ pipe_write(struct file *fp, struct uio *
if (error) {
--wpipe->pipe_busy;
if ((wpipe->pipe_busy == 0) &&
-   (wpipe->pipe_state & PIPE_WANT)) {
-   wpipe->pipe_state &= ~(PIPE_WANT | PIPE_WANTR);
+   (wpipe->pipe_state & PIPE_WANTD)) {
+   wpipe->pipe_state &= ~(PIPE_WANTD | PIPE_WANTR);
wakeup(wpipe);
}
goto done;
@@ -614,8 +614,8 @@ retrywrite:
 
--wpipe->pipe_busy;
 
-   if ((wpipe->pipe_busy == 0) && (wpipe->pipe_state & PIPE_WANT)) {
-   wpipe->pipe_state &= ~(PIPE_WANT | PIPE_WANTR);
+   if ((wpipe->pipe_busy == 0) && (wpipe->pipe_state & PIPE_WANTD)) {
+   wpipe->pipe_state &= ~(PIPE_WANTD | PIPE_WANTR);
wakeup(wpipe);
} else if (wpipe->pipe_buffer.cnt > 0) {
/*
@@ -810,7 +810,7 @@ pipeclose(struct pipe *cpipe)
cpipe->pipe_state |= PIPE_EOF;
while (cpipe->pipe_busy) {
wakeup(cpipe);
-   cpipe->pipe_state |= PIPE_WANT;
+   cpipe->pipe_state |= PIPE_WANTD;
tsleep(cpipe, PRIBIO, "pipecl", 0);
}
 
Index: sys/sys/pipe.h
===
--- sys/sys/pipe.h.orig 2019-07-12 14:58:51.733723987 +0200
+++ sys/sys/pipe.h  2019-07-13 07:07:00.450361707 +0200
@@ -61,7 +61,7 @@ struct pipebuf {
 #define PIPE_ASYNC 0x004   /* Async? I/O. */
 #define PIPE_WANTR 0x008   /* Reader wants some characters. */
 #define PIPE_WANTW 0x010   /* Writer wants space to put characters. */
-#define PIPE_WANT  0x020   /* Pipe is wanted to be run-down. */
+#define PIPE_WANTD 0x020   /* Pipe is wanted to be run-down. */
 #define PIPE_SEL   0x040   /* Pipe has a select active. */
 #define PIPE_EOF   0x080   /* Pipe is in EOF condition. */
 #define PIPE_LOCK  0x100   /* Process has exclusive access to 
pointers/data. */



pipe: revisit pipe initialisation and buffer allocation

2019-07-13 Thread Sebastien Marie
Hi,

The following diff revisits pipe initialiation (pipe_create() function)
and buffer management (pipespace() and pipe_free_kmem() functions).

Regarding pipe initialisation, get an already zeroed struct instead of
manually zeroing each struct members.

Rename pipespace() and pipe_free_kmem() to have more explicit name on
what the functions are for: reallocating or freeing the associated
buffer of the pipe. And it shows there are working on the same thing:
pipe_buffer_realloc() and pipe_buffer_free().

In pipe_free_kmem(), I changed the if-condition to return early instead
of having the whole function body inside the if-body.

No functional change intented.

Two KASSERT() added to pipespace(), one for the lock, another for
ensuring the buffer is empty before realloc.

Comments or OK ?
-- 
Sebastien Marie


Index: kern/sys_pipe.c
===
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.91
diff -u -p -r1.91 sys_pipe.c
--- kern/sys_pipe.c 13 Jul 2019 06:51:59 -  1.91
+++ kern/sys_pipe.c 13 Jul 2019 07:22:54 -
@@ -98,12 +98,13 @@ struct pool pipe_pool;
 
 intdopipe(struct proc *, int *, int);
 void   pipeclose(struct pipe *);
-void   pipe_free_kmem(struct pipe *);
 intpipe_create(struct pipe *);
 intpipelock(struct pipe *);
 void   pipeunlock(struct pipe *);
 void   pipeselwakeup(struct pipe *);
-intpipespace(struct pipe *, u_int);
+
+intpipe_buffer_realloc(struct pipe *, u_int);
+void   pipe_buffer_free(struct pipe *);
 
 /*
  * The pipe system call for the DTYPE_PIPE type of pipes
@@ -143,11 +144,11 @@ dopipe(struct proc *p, int *ufds, int fl
 
cloexec = (flags & O_CLOEXEC) ? UF_EXCLOSE : 0;
 
-   rpipe = pool_get(_pool, PR_WAITOK);
+   rpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
error = pipe_create(rpipe);
if (error != 0)
goto free1;
-   wpipe = pool_get(_pool, PR_WAITOK);
+   wpipe = pool_get(_pool, PR_WAITOK | PR_ZERO);
error = pipe_create(wpipe);
if (error != 0)
goto free1;
@@ -210,24 +211,30 @@ free1:
  * If it fails it will return ENOMEM.
  */
 int
-pipespace(struct pipe *cpipe, u_int size)
+pipe_buffer_realloc(struct pipe *cpipe, u_int size)
 {
caddr_t buffer;
 
+   /* buffer uninitialized or pipe locked */
+   KASSERT((cpipe->pipe_buffer.buffer == NULL) ||
+   (cpipe->pipe_state & PIPE_LOCK));
+
+   /* buffer should be empty */
+   KASSERT(cpipe->pipe_buffer.cnt == 0);
+
KERNEL_LOCK();
buffer = km_alloc(size, _any, _pageable, _waitok);
KERNEL_UNLOCK();
-   if (buffer == NULL) {
+   if (buffer == NULL)
return (ENOMEM);
-   }
 
/* free old resources if we are resizing */
-   pipe_free_kmem(cpipe);
+   pipe_buffer_free(cpipe);
+
cpipe->pipe_buffer.buffer = buffer;
cpipe->pipe_buffer.size = size;
cpipe->pipe_buffer.in = 0;
cpipe->pipe_buffer.out = 0;
-   cpipe->pipe_buffer.cnt = 0;
 
atomic_add_int(, cpipe->pipe_buffer.size);
 
@@ -242,19 +249,9 @@ pipe_create(struct pipe *cpipe)
 {
int error;
 
-   /* so pipe_free_kmem() doesn't follow junk pointer */
-   cpipe->pipe_buffer.buffer = NULL;
-   /*
-* protect so pipeclose() doesn't follow a junk pointer
-* if pipespace() fails.
-*/
-   memset(>pipe_sel, 0, sizeof(cpipe->pipe_sel));
-   cpipe->pipe_state = 0;
-   cpipe->pipe_peer = NULL;
-   cpipe->pipe_busy = 0;
sigio_init(>pipe_sigio);
 
-   error = pipespace(cpipe, PIPE_SIZE);
+   error = pipe_buffer_realloc(cpipe, PIPE_SIZE);
if (error != 0)
return (error);
 
@@ -461,7 +458,7 @@ pipe_write(struct file *fp, struct uio *
if ((npipe <= LIMITBIGPIPES) &&
(error = pipelock(wpipe)) == 0) {
if ((wpipe->pipe_buffer.cnt != 0) ||
-   (pipespace(wpipe, BIG_PIPE_SIZE) != 0))
+   (pipe_buffer_realloc(wpipe, BIG_PIPE_SIZE) != 0))
atomic_dec_int();
pipeunlock(wpipe);
} else
@@ -772,20 +769,29 @@ pipe_close(struct file *fp, struct proc 
return (0);
 }
 
+/*
+ * Free kva for pipe circular buffer.
+ * No lock check as only called from pipe_buffer_realloc() and pipeclose()
+ */
 void
-pipe_free_kmem(struct pipe *cpipe)
+pipe_buffer_free(struct pipe *cpipe)
 {
-   u_int size = cpipe->pipe_buffer.size;
+   u_int size;
 
-   if (cpipe->pipe_buffer.buffer != NULL) {
-   KERNEL_LOCK();
-   km_free(cpipe->pipe_buffer.buffer, size, _any, _pageable);
-   KERNEL_UNLOCK();
-   atomic_sub_int(, size);
-   cpipe->pipe_buffer.buffer = 

pipe(2) and pipe2(2) : nolock

2019-06-24 Thread Sebastien Marie
Hi,

The following diff sets NOLOCK on dup(2) and dup2(2) syscalls. It is a
part of "unlock more syscalls".

All the hard work was already done by mpi@. My changes are mostly to add
few KASSERT() or comments in current code.

The key function for pipe creation is dopipe(). It creates 2 pipes
struct using pipecreate(), allocate descriptors and insert them (this
part is already KERNEL_LOCK safe).

The specific part are pipe_create() and error code which call pipeclose().

1. pipe_create()

pipe_create() do only initialization with safe code, or already take the
KERNEL_LOCK like for km_alloc() or km_free().

It also call pipespace() to initialize the buffer. I added some
KASSERT() inside it to make visible the expectation: the pipe should
be uninitialized (like when called from pipe_create() or locked (with
PIPE_LOCK, as the buffer will be freed and realloced).

While here, I also added a KASSERT() to stand the buffer is empty before
replacing it.


2. pipeclose()

In the general case, pipeclose() isn't safe to be called without KERNEL_LOCK:
- if the pipe is busy, it could call wakeup() and tsleep()
- if the pipe has peer, it could call pipeselwakeup() and wakeup()

But we are in dopipe(), so it is a special case:
- the pipe isn't used for now, so pipe_busy couldn't be set
- the error code paths possibility calling pipeclose() are before setting
  pipe_peer, so the pipe has not peer when called.

So I added a comment to say it is safe to call pipeclose() here.



And a side question: what is the purpose of `amountpipekva' variable ?
it is incremented/decremented to reflect the total size of all pipe
buffers in use, but it is used nowhere outside pipespace() for inc
and pipe_free_kmem() for dec. should be deleted or it is useful for
debugging (via ddb perhaps) ?



For testing, please regenerate syscalls after applying the diff, and
before compiling the kernel:

$ cd /sys/kern && make syscalls


Comments or OK ?
-- 
Sebastien Marie



Index: kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.192
diff -u -p -r1.192 syscalls.master
--- kern/syscalls.master22 Jun 2019 06:49:14 -  1.192
+++ kern/syscalls.master24 Jun 2019 10:26:14 -
@@ -216,7 +216,7 @@
socklen_t namelen); }
 99 STD { int sys_getdents(int fd, void *buf, size_t buflen); }
 100STD { int sys_getpriority(int which, id_t who); }
-101STD { int sys_pipe2(int *fdp, int flags); }
+101STD NOLOCK  { int sys_pipe2(int *fdp, int flags); }
 102STD { int sys_dup3(int from, int to, int flags); }
 103STD { int sys_sigreturn(struct sigcontext *sigcntxp); }
 104STD { int sys_bind(int s, const struct sockaddr *name, \
@@ -448,7 +448,7 @@
 260UNIMPL
 261UNIMPL
 262UNIMPL
-263STD { int sys_pipe(int *fdp); }
+263STD NOLOCK  { int sys_pipe(int *fdp); }
 264STD { int sys_fhopen(const fhandle_t *fhp, int flags); }
 265UNIMPL
 266UNIMPL
Index: kern/sys_pipe.c
===
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.88
diff -u -p -r1.88 sys_pipe.c
--- kern/sys_pipe.c 22 Jun 2019 06:48:25 -  1.88
+++ kern/sys_pipe.c 24 Jun 2019 10:30:12 -
@@ -198,6 +198,7 @@ free3:
 free2:
fdpunlock(fdp);
 free1:
+   /* fine without KERNEL_LOCK because just created */
pipeclose(wpipe);
pipeclose(rpipe);
return (error);
@@ -214,12 +215,18 @@ pipespace(struct pipe *cpipe, u_int size
 {
caddr_t buffer;
 
+   /* pipe should be uninitialized or locked */
+   KASSERT((cpipe->pipe_buffer.buffer == NULL) ||
+   (cpipe->pipe_state & PIPE_LOCK));
+
+   /* buffer should be empty */
+   KASSERT(cpipe->pipe_buffer.cnt == 0);
+
KERNEL_LOCK();
buffer = km_alloc(size, _any, _pageable, _waitok);
KERNEL_UNLOCK();
-   if (buffer == NULL) {
+   if (buffer == NULL)
return (ENOMEM);
-   }
 
/* free old resources if we are resizing */
pipe_free_kmem(cpipe);
@@ -227,7 +234,6 @@ pipespace(struct pipe *cpipe, u_int size
cpipe->pipe_buffer.size = size;
cpipe->pipe_buffer.in = 0;
cpipe->pipe_buffer.out = 0;
-   cpipe->pipe_buffer.cnt = 0;
 
atomic_add_int(, cpipe->pipe_buffer.size);
 
@@ -244,6 +250,8 @@ pipe_create(struct pipe *cpipe)
 
/* so pipe_free_kmem() doesn't follow junk pointer */
cpipe->pipe_buffer.buffer = NULL;
+   cpipe->pipe_buffer.cnt = 0;
+
/*
 * protect so pipeclose() doesn't follow a junk pointer
 * if pipespace() fails.
@@ -303,6 +311,7 @@ pipeselwakeup(struct pipe *cpipe)
selwakeup(>pipe_sel);
  

Re: sppp: remove duplicate initialisation

2019-06-22 Thread Sebastien Marie
On Sat, Jun 22, 2019 at 11:29:42AM +0200, Klemens Nanni wrote:
> OK?

OK semarie@
 
> Index: net/if_spppsubr.c
> ===
> RCS file: /cvs/src/sys/net/if_spppsubr.c,v
> retrieving revision 1.174
> diff -u -p -r1.174 if_spppsubr.c
> --- net/if_spppsubr.c 19 Feb 2018 08:59:52 -  1.174
> +++ net/if_spppsubr.c 22 Jun 2019 09:28:17 -
> @@ -3566,7 +3562,6 @@ sppp_chap_tlu(struct sppp *sp)
>   STDDCL;
>   int i = 0, x;
>  
> - i = 0;
>   sp->rst_counter[IDX_CHAP] = sp->lcp.max_configure;
>  
>   /*
> 

-- 
Sebastien Marie



Re: free(9) size & mallocaray for M_IPMOPTS

2019-06-09 Thread Sebastien Marie
  immp = (struct in_multi **)malloc(
> - (sizeof(*immp) * IP_MIN_MEMBERSHIPS), M_IPMOPTS,
> + immp = mallocarray(IP_MIN_MEMBERSHIPS, sizeof(*immp), M_IPMOPTS,
>   M_WAITOK|M_ZERO);
>   *imop = imo;
>   imo->imo_ifidx = 0;
> @@ -1517,9 +1516,8 @@ ip_setmoptions(int optname, struct ip_mo
>   omships = imo->imo_membership;
>   newmax = ((imo->imo_max_memberships + 1) * 2) - 1;
>   if (newmax <= IP_MAX_MEMBERSHIPS) {
> - nmships = (struct in_multi **)mallocarray(
> - newmax, sizeof(*nmships), M_IPMOPTS,
> - M_NOWAIT|M_ZERO);
> + nmships = mallocarray(newmax, sizeof(*nmships),
> + M_IPMOPTS, M_NOWAIT|M_ZERO);
>   if (nmships != NULL) {
>   memcpy(nmships, omships,
>   sizeof(*omships) *
> @@ -1623,7 +1621,8 @@ ip_setmoptions(int optname, struct ip_mo
>   imo->imo_ttl == IP_DEFAULT_MULTICAST_TTL &&
>   imo->imo_loop == IP_DEFAULT_MULTICAST_LOOP &&
>   imo->imo_num_memberships == 0) {
> - free(imo->imo_membership , M_IPMOPTS, 0);
> + free(imo->imo_membership , M_IPMOPTS,
> + imo->imo_max_memberships * sizeof(struct in_multi *));
>   free(*imop, M_IPMOPTS, sizeof(**imop));
>   *imop = NULL;
>   }
> @@ -1688,7 +1687,8 @@ ip_freemoptions(struct ip_moptions *imo)
>   if (imo != NULL) {
>   for (i = 0; i < imo->imo_num_memberships; ++i)
>   in_delmulti(imo->imo_membership[i]);
> - free(imo->imo_membership, M_IPMOPTS, 0);
> + free(imo->imo_membership, M_IPMOPTS,
> + imo->imo_max_memberships * sizeof(struct in_multi *));
>   free(imo, M_IPMOPTS, sizeof(*imo));
>   }
>  }
> Index: netinet6/ip6_output.c
> ===
> RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> retrieving revision 1.243
> diff -u -p -r1.243 ip6_output.c
> --- netinet6/ip6_output.c 28 Apr 2019 22:15:58 -  1.243
> +++ netinet6/ip6_output.c 9 Jun 2019 17:35:15 -
> @@ -1882,9 +1882,7 @@ ip6_setmoptions(int optname, struct ip6_
>* No multicast option buffer attached to the pcb;
>* allocate one and initialize to default values.
>*/
> - im6o = (struct ip6_moptions *)
> - malloc(sizeof(*im6o), M_IPMOPTS, M_WAITOK);
> -
> + im6o = malloc(sizeof(*im6o), M_IPMOPTS, M_WAITOK);
>   if (im6o == NULL)
>   return (ENOBUFS);
>   *im6op = im6o;
> @@ -2138,7 +2136,7 @@ ip6_setmoptions(int optname, struct ip6_
>   im6o->im6o_hlim == ip6_defmcasthlim &&
>   im6o->im6o_loop == IPV6_DEFAULT_MULTICAST_LOOP &&
>   LIST_EMPTY(>im6o_memberships)) {
> - free(*im6op, M_IPMOPTS, 0);
> + free(*im6op, M_IPMOPTS, sizeof(*im6o));
>   *im6op = NULL;
>   }
>  
> @@ -2202,7 +2200,7 @@ ip6_freemoptions(struct ip6_moptions *im
>   LIST_REMOVE(imm, i6mm_chain);
>   in6_leavegroup(imm);
>   }
> - free(im6o, M_IPMOPTS, 0);
> + free(im6o, M_IPMOPTS, sizeof(*im6o));
>  }
>  
>  /*
> 

-- 
Sebastien Marie



[patch] push the KERNEL_LOCK deeper on read(2) and write(2)

2019-06-05 Thread Sebastien Marie
Hi,

I would like to have feedback and testing on this diff. The initial work
was done by ian@.

It unlocks read(2) and write(2) families, and push the KERNEL_LOCK
deeper in the code path. With it, read(2) and write(2) on socket will be
KERNEL_LOCK-free.

read(2) and write(2) are file type agnostics by using `struct fileops'
(see sys/file.h) which define effective implementation per file type.

Currently, we have the following fileops defined:
- dmabufdev/pci/drm/drm_linux.c
- kqueuekern/kern_event.c
- pipe  kern/sys_pipe.c
- socketkern/sys_socket.c
- vnode kern/vfs_vnops.c


read(2) family uses dofilereadv(), which calls:
- fileops::fo_read()
- FRELE() -> fdrop() -> fileops::fo_close()

write(2) family uses dofilewritev(), which calls:
- fileops::fo_write()
- FRELE() -> fdrop() -> fileops::fo_close()


The diff pushs the KERNEL_LOCK inside each function, where it is
effectively needed. But as some file types doesn't need the lock (socket
for example), it makes read(2) and write(2) lock-free for them, while
still grabbing the lock for others (vnode for example).

fileops::fo_read
- dmabuf_read   fine lock-free (just return ENXIO)
- kqueue_read   fine lock-free (just return ENXIO)
- pipe_read KERNEL_LOCK/UNLOCK added (tsleep, wakeup, selwakeup)
- soo_read  calls soreceive() which is already called without KERNEL_LOCK 
by recvit() (via recvmsg(2))
- vn_read   KERNEL_LOCK/UNLOCK added

fileops::fo_write
- dmabuf_write  fine lock-free (just return ENXIO)
- kqueue_write  fine lock-free (just return ENXIO)
- pipe_writeKERNEL_LOCK/UNLOCK added (tsleep, wakeup, selwakeup)
- soo_write calls sosend() which is already called without KERNEL_LOCK by 
sendit() (via sendmsg(2))
- vn_write  KERNEL_LOCK/UNLOCK added

fileops::fo_close
- dmabuf_close  already take care of it
- kqueue_close  already take care of it
- pipe_closealready take care of it
- soo_close call soclose() which is already called without KERNEL_LOCK by 
socketpair(2)
- vn_closefile  already take care of it


In dofilewritev(), I take care of calling ptsignal() with the
KERNEL_LOCK() too, as it requires the lock (it is asserted).

Others functions should be fine to be called without the KERNEL_LOCK, as
they are already used in such context.

Thanks.
-- 
Sebastien Marie


Index: kern/sys_generic.c
===
RCS file: /cvs/src/sys/kern/sys_generic.c,v
retrieving revision 1.123
diff -u -p -r1.123 sys_generic.c
--- kern/sys_generic.c  21 Jan 2019 23:41:26 -  1.123
+++ kern/sys_generic.c  4 Jun 2019 06:19:23 -
@@ -366,8 +366,11 @@ dofilewritev(struct proc *p, int fd, str
if (uio->uio_resid != cnt && (error == ERESTART ||
error == EINTR || error == EWOULDBLOCK))
error = 0;
-   if (error == EPIPE)
+   if (error == EPIPE) {
+   KERNEL_LOCK();
ptsignal(p, SIGPIPE, STHREAD);
+   KERNEL_UNLOCK();
+   }
}
cnt -= uio->uio_resid;
 
Index: kern/sys_pipe.c
===
RCS file: /cvs/src/sys/kern/sys_pipe.c,v
retrieving revision 1.87
diff -u -p -r1.87 sys_pipe.c
--- kern/sys_pipe.c 13 Nov 2018 13:02:20 -  1.87
+++ kern/sys_pipe.c 5 Jun 2019 08:06:19 -
@@ -314,9 +314,11 @@ pipe_read(struct file *fp, struct uio *u
int error;
size_t size, nread = 0;
 
+   KERNEL_LOCK();
+
error = pipelock(rpipe);
if (error)
-   return (error);
+   goto done;
 
++rpipe->pipe_busy;
 
@@ -420,6 +422,8 @@ unlocked_error:
if ((rpipe->pipe_buffer.size - rpipe->pipe_buffer.cnt) >= PIPE_BUF)
pipeselwakeup(rpipe);
 
+done:
+   KERNEL_UNLOCK();
return (error);
 }
 
@@ -430,6 +434,8 @@ pipe_write(struct file *fp, struct uio *
size_t orig_resid;
struct pipe *wpipe, *rpipe;
 
+   KERNEL_LOCK();
+
rpipe = fp->f_data;
wpipe = rpipe->pipe_peer;
 
@@ -437,7 +443,8 @@ pipe_write(struct file *fp, struct uio *
 * detect loss of pipe read side, issue SIGPIPE if lost.
 */
if ((wpipe == NULL) || (wpipe->pipe_state & PIPE_EOF)) {
-   return (EPIPE);
+   error = EPIPE;
+   goto done;
}
++wpipe->pipe_busy;
 
@@ -471,7 +478,7 @@ pipe_write(struct file *fp, struct uio *
wpipe->pipe_state &= ~(PIPE_WANT | PIPE_WANTR);
wakeup(wpipe);
}
-   return (error);
+   goto done;
}
 
orig_resid = uio->uio_resid;
@@ -642,6 +649,8 @@ retrywrite:
if (wpipe->pipe_buffer.cnt)
pipeselwakeup(wpipe);
 
+done:
+   KERNEL

Re: stack trace / free(0) in isascan()

2019-05-09 Thread Sebastien Marie
On Thu, May 09, 2019 at 10:55:44AM +0200, Hrvoje Popovski wrote:
> 
> with this diff i'm getting new traces

it is (somehow) expected.

the commit that starts showing traces do the following:
- when there is missing size on free() reports it (with a backtrace to know the 
caller)
- but report only a fixed number of calls (5), because else users will be mad

so by correcting some sizes it makes others calls to free() to be visible.

> free with zero size: (127)
> Starting stack trace...
> free(8013f800,7f,0,8013f800,cf43c4f465ef43f8,0) at free+0xd8
> uhidev_attach(80071200,8014ed00,8000224a40a0,80071200,89eb6e07df884e85,80071200)
>  at uhidev_attach+0x1b4

> free with zero size: (127)
> Starting stack trace...
> free(8013f800,7f,0,8013f800,cf43c4f465284bc3,0) at free+0xd8
> hid_report_size(80070c00,41,0,0,764c887b264d079f,0) at 
> hid_report_size+0x10f

> free with zero size: (127)
> Starting stack trace...
> free(8013f800,7f,0,8013f800,cf43c4f465284185,0) at free+0xd8
> hid_is_collection(80070c00,41,ff,10006,a6cb281b8426ee7e,81cf60e0)
>  at hid_is_collection+0xe9

> free with zero size: (127)
> Starting stack trace...
> free(8013f800,7f,0,8013f800,cf43c4f465284185,0) at free+0xd8
> hid_is_collection(80070c00,41,ff,10001,a6cb281b844568dc,81cf6118)
>  at hid_is_collection+0xe9

> free with zero size: (127)
> Starting stack trace...
> free(8013f800,7f,0,8013f800,cf43c4f465284185,0) at free+0xd8
> hid_is_collection(80070c00,41,ff,10002,a6cb281b844568fc,3) at 
> hid_is_collection+0xe9

I am leaving others free() calls to people that would like to play this game 
too.
-- 
Sebastien Marie



Re: stack trace / free(0) in isascan()

2019-05-09 Thread Sebastien Marie
On Thu, May 09, 2019 at 10:12:49AM +0200, Hrvoje Popovski wrote:
> Hi all,
> 
> i update kernel from cvs few minutes ago and i'm seeing this stack trace
> in dmesg. i'm just reporting it.

The principe of the game is to look at the free() call that still use 0
as size, and found the right size to fill it.

> free with zero size: (2)
> Starting stack trace...
> free(80074100,2,0,80074100,1d5a05b47900fbf4,81d287e8) 
> at free+0xd8
> isascan(80101200,80074100,9563e4d9af15796a,81618690,80101200,81d16d01)
>  at isascan+0x3f8

Here, isascan() calls free() on dev (a struct device).

the related malloc() call is done by config_make_softc() at line 248:
   247  config_attach(parent, dev, , isaprint);
   248  dev = config_make_softc(parent, cf);

The size used for malloc() is :

kern/subr_autoconf.c
   417  struct device *
   418  config_make_softc(struct device *parent, struct cfdata *cf)
   419  {
   420  struct device *dev;
   421  struct cfdriver *cd;
   422  struct cfattach *ca;
   423
   424  cd = cf->cf_driver;
   425  ca = cf->cf_attach;
   426  if (ca->ca_devsize < sizeof(struct device))
   427  panic("config_make_softc");
   428
   429  /* get memory for all device vars */
   430  dev = malloc(ca->ca_devsize, M_DEVBUF, M_NOWAIT|M_ZERO);
   431  if (dev == NULL)
   432  panic("config_make_softc: allocation for device softc 
failed");


So calling free() with cf->cf_attach->ca_devsize should be fine.
Diff below.

OK ?
-- 
Sebastien Marie

Index: dev/isa/isa.c
===
RCS file: /cvs/src/sys/dev/isa/isa.c,v
retrieving revision 1.46
diff -u -p -r1.46 isa.c
--- dev/isa/isa.c   25 May 2015 15:19:22 -  1.46
+++ dev/isa/isa.c   9 May 2019 08:28:47 -
@@ -257,7 +257,7 @@ isascan(parent, match)
if (autoconf_verbose)
printf(">>> probing for %s* finished\n",
cf->cf_driver->cd_name);
-   free(dev, M_DEVBUF, 0);
+   free(dev, M_DEVBUF, cf->cf_attach->ca_devsize);
return;
}
 
@@ -270,7 +270,7 @@ isascan(parent, match)
!isa_intr_check(sc->sc_ic, ia.ia_irq, IST_EDGE)) {
printf("%s%d: irq %d already in use\n",
cf->cf_driver->cd_name, cf->cf_unit, ia.ia_irq);
-   free(dev, M_DEVBUF, 0);
+   free(dev, M_DEVBUF, cf->cf_attach->ca_devsize);
} else {
 #endif
if (autoconf_verbose)
@@ -291,7 +291,7 @@ isascan(parent, match)
if (autoconf_verbose)
printf(">>> probing for %s%d failed\n",
cf->cf_driver->cd_name, cf->cf_unit);
-   free(dev, M_DEVBUF, 0);
+   free(dev, M_DEVBUF, cf->cf_attach->ca_devsize);
}
 }
 



Re: dwxe: resetting interface on watchdog timeout

2019-04-17 Thread Sebastien Marie
On Wed, Apr 17, 2019 at 04:32:04PM -0700, Jungle Boogie wrote:
> On Wed 17 Apr 2019  9:44 AM, Sebastien Marie wrote:
> > Hi,
> > 
> > With a pine64, I am experimenting regulary dwxe watchdog
> > timeout. Usually it is a sign that something doesn't work in the driver
> > itself.
> 
> Good to know this isn't just affecting my three devices.
> Let's hope this patch gets some feedback and makes its way into the build.

you could build a kernel and test it for confirming it works as expected.

it could really help to have feedback from users.

thanks.
-- 
Sebastien Marie



dwxe: resetting interface on watchdog timeout

2019-04-17 Thread Sebastien Marie
Hi,

With a pine64, I am experimenting regulary dwxe watchdog
timeout. Usually it is a sign that something doesn't work in the driver
itself.

The problem I am facing currently is when watchdog timeout occurs,
the interface is unusable. And so I need another system connected
permanently to serial in order to login and reboot the board to get it
working.

The following diff is still a workaround for the underline driver
problem. It tries to reset the interface when watchdog timeout
occurs. But at least, the board could come back in a more accessible
state.

When a watchdog timeout occurs, it will try to:
- down the interface (if it is up)
- reset it
- up the interface (if it called down previously)

With it, I have a "stable" connection to the board via network.

Comments or OK ?
-- 
Sebastien Marie


Index: if_dwxe.c
===
RCS file: /cvs/src/sys/dev/fdt/if_dwxe.c,v
retrieving revision 1.11
diff -u -p -r1.11 if_dwxe.c
--- if_dwxe.c   3 Jan 2019 00:59:58 -   1.11
+++ if_dwxe.c   15 Apr 2019 10:21:39 -
@@ -687,7 +687,21 @@ dwxe_ioctl(struct ifnet *ifp, u_long cmd
 void
 dwxe_watchdog(struct ifnet *ifp)
 {
-   printf("%s\n", __func__);
+   struct dwxe_softc *sc = ifp->if_softc;
+   int down_up = 0;
+
+   printf("%s: watchdog timeout\n", sc->sc_dev.dv_xname);
+   ifp->if_oerrors++;
+
+   if (ifp->if_flags & IFF_RUNNING) {
+   down_up = 1;
+   dwxe_down(sc);
+   }
+
+   dwxe_reset(sc);
+
+   if (down_up == 1)
+   dwxe_up(sc);
 }
 
 int



-msave-args : uninitialized variable

2019-02-04 Thread Sebastien Marie
Hi,

Recently, devel/llvm (the port) has copied the -msave-args diff from
base, and it resulted lang/rust to segfault.

Since, the change has been backouted, but I continued searching the root
cause as the diff is on base too.

and I think the culprit is SaveArgs variable member in class X86Subtarget.

If -msave-args is used, it is set to `true'.
If -mno-save-args is used, it is set to `false'.
But else, it lefts uninitialized.

As the value is a boolean, I changed type from unsigned to bool, and set
the default value.

I didn't test it on base, but with such patch on devel/llvm, rust is
able to compile correctly.

-- 
Sebastien Marie

Index: lib/Target/X86/X86Subtarget.h
===
RCS file: /cvs/src/gnu/llvm/lib/Target/X86/X86Subtarget.h,v
retrieving revision 1.4
diff -u -p -r1.4 X86Subtarget.h
--- lib/Target/X86/X86Subtarget.h   30 Jan 2019 03:08:12 -  1.4
+++ lib/Target/X86/X86Subtarget.h   4 Feb 2019 15:44:11 -
@@ -401,7 +401,7 @@ protected:
   unsigned stackAlignment = 4;
 
   /// Whether function prologues should save register arguments on the stack.
-  unsigned SaveArgs;
+  bool SaveArgs = false;
 
   /// Max. memset / memcpy size that is turned into rep/movs, rep/stos ops.
   ///
@@ -481,7 +481,7 @@ public:
 return ()->getRegisterInfo();
   }
 
-  unsigned getSaveArgs() const { return SaveArgs; }
+  bool getSaveArgs() const { return SaveArgs; }
 
   /// Returns the minimum alignment known to hold of the
   /// stack frame on entry to the function and which must be maintained by 
every



add /etc/unwind.conf to changelist(5)

2019-01-29 Thread Sebastien Marie
Hi,

Does it make sens to add unwind.conf to changelist ?

I am not proposing /etc/unwind/trustanchor/root.key as the content
change regulary (it has `last_queried' information in comment for
example).

Thanks.
-- 
Sebastien Marie

Index: changelist
===
RCS file: /cvs/src/etc/changelist,v
retrieving revision 1.122
diff -u -p -r1.122 changelist
--- changelist  23 Jul 2018 11:57:17 -  1.122
+++ changelist  29 Jan 2019 10:02:13 -
@@ -135,6 +135,7 @@
 /etc/switchd.conf
 /etc/sysmerge.ignore
 /etc/ttys
+/etc/unwind.conf
 /etc/usermgmt.conf
 /etc/vm.conf
 /etc/weekly



Re: unveil file(1)

2019-01-04 Thread Sebastien Marie
Hi,

First, for the specific case of file(1), I agree that calling "rpath" is what
to do. No point to use unveil("/", "r").

On Thu, Jan 03, 2019 at 06:34:06PM -0700, Theo de Raadt wrote:
> 
> You are abusing kernel resources.
> 

if a userland program could be be crafted to abuse kernel resources, I
would saying that userland is able to DoS the kernel in some extents,
and it could be a problem on multi-users systems.

Just saying "you should not do that" will not prevent any user to do it
anyway.

Currently, unveil(2) has a static UNVEIL_MAX_VNODES restriction (128
vnodes allocated by proc). As, when considering a pathname, you are
holding the vnode for each compoment of the path, it could be hitted
quickly ("/a/path/to/file" is 5 vnodes).

If we want resource limitation to avoid abusing from userland,
setrlimit(2) could be used. But it is per process limitation. For
example, we could account the held vnode in RLIMIT_NOFILE. But I am
unsure it would be useful: the UNVEIL_MAX_VNODES limitation is lower
that default RLIMIT_NOFILES in `default' class (512).

And trying to resolv a global resource consumption (number of vnodes
held) at process level doesn't seems right (fork could be used to avoid
the per-process restricition).

But, is the situation with unveil(2) worst that before ? It seems to me
a user could already doing vnode comsuption with just opening as many
file descriptors it can (and fork and repeat when RLIMIT_NOFILES is
reached for the process).

Eventually, with one unveil(2) call, you could held more vnode than with
one open(2) call. So it is cheaper for attacker to use unveil(2) instead
of use open(2).

So I am unsure if the problem is really "abusing kernel resources". It
is more "you can't use naively unveil(2) on user provided list" bcause
your program has more chance to quit unexpectly (so eventually something
to clarify in unveil(2) man page ?)

As for any system-call, userland using it has to be ready to copte with
error code. And for unveil(2), with E2BIG error code in particular (per
unveil(2) man page: "The addition of path would exceed the per-process
limit for unveiled paths."). But as you can't undo unveil once called,
it should be decided before calling it, and it could be not trivial in
some cases. Failing to do it usually mean the program will die as the
usual code when using unveil(2) is:

if (unveil(path, perm) == -1)
err(EXIT_FAILURE, "unveil");

Regards.
-- 
Sebastien Marie



Re: video(1) pledge (& updated kernel diff)

2018-12-30 Thread Sebastien Marie
On Sun, Dec 30, 2018 at 10:58:58AM +0100, Landry Breuil wrote:
> On Sat, Dec 29, 2018 at 09:30:22AM +0100, Sebastien Marie wrote:
> > On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
> 
> > I think you pledged too early.
> > 
> > "dns unix" are here for XOpenDisplay(), but if the display is remote,
> > "inet" could be needed too. Even if Xv isn't possible on remote display,
> > XOpenDisplay() will be killed by pledge.
> > 
> > So ideally, pledge(2) should occurs later.
> 
> Yup, after some discussions, moving the pledge calls after setup()
> (which does the file & xv opening) allows to only need stdio in the -i
> case, and 'stdio rpath video' in the others. i thought rpath could be
> dropped but in my testing with video -vvv -O i had a crash upon some
> keys where x tried to open /usr/X11R6/share/X11/locale/locale.alias..

libX11 tentacles :)  I think any program using X11 needs rpath.
 
> in addition we can also pledge the video -q case which needs
> stdio/rpath/wpath/video.

The promises looks more sanely now.

For me, there is still initialization stuff in dev_check_caps(), a
function used to query capabilities of the device (if the device is able
to record video, and how [via read-write or via streaming (mmap)]).

As it is the first function using ioctl(2) on the device, it opens
the descriptor device (using O_RDWR). It is why we need rpath+wpath
here. But such refactoring is outside the scope of the diff. No problem
with that.

Thanks. I will review the kernel side soon.

> Index: video.c
> ===
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.25
> diff -u -r1.25 video.c
> --- video.c   9 Apr 2018 18:16:44 -   1.25
> +++ video.c   30 Dec 2018 09:39:27 -
> @@ -1961,6 +1961,8 @@
>   argv += optind;
>  
>   if (vid.mode & M_QUERY) {
> + if (pledge("stdio rpath wpath video", NULL) == -1)
> + err(1, "pledge");
>   dev_dump_query();
>   cleanup(, 0);
>   }
> @@ -1970,6 +1972,14 @@
>  
>   if (!setup())
>   cleanup(, 1);
> +
> + if (vid.mode & M_IN_FILE) {
> + if (pledge("stdio", NULL) == -1)
> + err(1, "pledge");
> + } else {
> + if (pledge("stdio rpath video", NULL) == -1)
> + err(1, "pledge");
> + }
>  
>   if (!stream())
>   cleanup(, 1);


-- 
Sebastien Marie



Re: video(1) pledge (& updated kernel diff)

2018-12-30 Thread Sebastien Marie
On Sun, Dec 30, 2018 at 10:58:58AM +0100, Landry Breuil wrote:
> On Sat, Dec 29, 2018 at 09:30:22AM +0100, Sebastien Marie wrote:
> > On Fri, Dec 28, 2018 at 09:41:06PM +0100, Landry Breuil wrote:
> > 
> > I would separate the addition of pledge(2) and unrelated fixes.
> 
> Right, two separated diffs attached for this.
> 

the err->errs diff to avoid shadowing is ok semarie@

mherrb@, any objections to it ?

Thanks
-- 
Sebastien Marie

> Index: video.c
> ===
> RCS file: /cvs/xenocara/app/video/video.c,v
> retrieving revision 1.25
> diff -u -r1.25 video.c
> --- video.c   9 Apr 2018 18:16:44 -   1.25
> +++ video.c   30 Dec 2018 09:39:21 -
> @@ -1854,7 +1854,7 @@
>   struct xdsp *x = 
>   const char *errstr;
>   size_t len;
> - int ch, err = 0;
> + int ch, errs = 0;
>  
>   bzero(, sizeof(struct video));
>  
> @@ -1872,21 +1872,21 @@
>   x->cur_adap = strtonum(optarg, 0, 4, );
>   if (errstr != NULL) {
>   warnx("Xv adaptor '%s' is %s", optarg, errstr);
> - err++;
> + errs++;
>   }
>   break;
>   case 'e':
>   vid.enc = find_enc(optarg);
>   if (vid.enc >= ENC_LAST) {
>   warnx("encoding '%s' is invalid", optarg);
> - err++;
> + errs++;
>   }
>   break;
>   case 'f':
>   len = strlcpy(d->path, optarg, sizeof(d->path));
>   if (len >= sizeof(d->path)) {
>   warnx("file path is too long: %s", optarg);
> - err++;
> + errs++;
>   }
>   break;
>   case 'g':
> @@ -1894,8 +1894,8 @@
>   break;
>   case 'i':
>   if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
> - warnx("only one input or ouput file allowed");
> - err++;
> + warnx("only one input or output file allowed");
> + errs++;
>   } else {
>   vid.mode = (vid.mode & ~M_IN_DEV) | M_IN_FILE;
>   vid.mmap_on = 0; /* mmap mode does not work for 
> files */
> @@ -1904,15 +1904,15 @@
>   if (len >= sizeof(vid.iofile)) {
>   warnx("input path is too long: %s",
>   optarg);
> - err++;
> + errs++;
>   }
>   }
>   break;
>   case 'o':
>   case 'O':
>   if (vid.mode & (M_IN_FILE | M_OUT_FILE)) {
> - warnx("only one input or ouput file allowed");
> - err++;
> + warnx("only one input or output file allowed");
> + errs++;
>   } else {
>   vid.mode |= M_OUT_FILE;
>   if (ch != 'O')
> @@ -1922,7 +1922,7 @@
>   if (len >= sizeof(vid.iofile)) {
>   warnx("output path is too long: %s",
>   optarg);
> - err++;
> + errs++;
>   }
>   }
>   break;
> @@ -1937,7 +1937,7 @@
>   vid.fps = strtonum(optarg, 1, 100, );
>   if (errstr != NULL) {
>   warnx("frame rate '%s' is %s", optarg, errstr);
> - err++;
> + errs++;
>   }
>   break;
>   case 's':
> @@ -1947,13 +1947,13 @@
>   vid.verbose++;
>   break;
>   default:
> - err++;
> + errs++;
>   break;
>   }
> - if (err > 0)
> + if (errs > 0)
>   break;
>   }
> - if (err > 0) {
> + if (errs > 0) {
>   usage();
>   cleanup(, 1);
>   }



Re: video(1) pledge (& updated kernel diff)

2018-12-29 Thread Sebastien Marie
+ case VIDIOC_STREAMOFF:
> + case VIDIOC_ENUM_FRAMESIZES:
> + case VIDIOC_ENUM_FRAMEINTERVALS:

I didn't check them for now. I will do it later.

Please note that I will take in consideration all ioctls provided by
video(4), and not only the one used by video(1) or firefox. The purpose
is to have usable "video" promise, without been widely opened neither
too restricted. It is why we should considere several programs at once
when designing the promise, and look at kernel code path to have the
attack surface opened by the promise.

> + if (fp->f_type == DTYPE_VNODE &&
> + vp->v_type == VCHR &&
> + cdevsw[major(vp->v_rdev)].d_open == videoopen)
> + return (0);
> + break;
> + }
> + }
> +
>  
>  #if NPF > 0
>   if ((p->p_p->ps_pledge & PLEDGE_PF)) {
> Index: sys/pledge.h
> ===
> RCS file: /cvs/src/sys/sys/pledge.h,v
> retrieving revision 1.38
> diff -u -r1.38 pledge.h
> --- sys/pledge.h  11 Aug 2018 16:16:07 -  1.38
> +++ sys/pledge.h  28 Dec 2018 20:23:02 -
> @@ -62,6 +62,7 @@
>  #define PLEDGE_ERROR 0x0004ULL   /* ENOSYS instead of kill */
>  #define PLEDGE_WROUTE0x0008ULL   /* interface address 
> ioctls */
>  #define PLEDGE_UNVEIL0x0010ULL   /* allow unveil() */
> +#define PLEDGE_VIDEO 0x00200800ULL   /* video ioctls */

I suspect a copy/paste error: PLEDGE_VIDEO contains 2 bits sets (there is a '8' 
after the '2').
and space vs tab between the name and the value.
  
>  /*
>   * Bits outside PLEDGE_USERSET are used by the kernel itself
> @@ -111,6 +112,7 @@
>   { PLEDGE_ERROR, "error" },
>   { PLEDGE_WROUTE,"wroute" },
>   { PLEDGE_UNVEIL,"unveil" },
> + { PLEDGE_VIDEO, "video" },
>   { 0, NULL },
>  };
>  #endif

Thanks for the recall.
-- 
Sebastien Marie



Re: pwd_check tweak

2018-12-09 Thread Sebastien Marie
On Sun, Dec 09, 2018 at 09:14:38PM -0500, Ted Unangst wrote:
> These patterns try to detect a1a1a1 style passwords. By making the regex a bit
> more flexible we can just use one. Also now catches mMmMmM fwiw.

it will also catches any password composed of only letters and digits
from 2 to 8 chars (need even numbers of chars).

like: aRgh675P or 78Ytgs7A

but I am unsure if it is bad or not. I think any password with only 8
chars is bad now.

> Index: pwd_check.c
> ===
> RCS file: /cvs/src/usr.bin/passwd/pwd_check.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 pwd_check.c
> --- pwd_check.c   21 Aug 2017 21:41:13 -  1.16
> +++ pwd_check.c   10 Dec 2018 02:09:51 -
> @@ -72,15 +72,10 @@ struct pattern patterns[] = {
>   "Please use a more complicated password."
>   },
>   {
> - "^([a-z][0-9]){1,4}$",
> + "^([a-z0-9][a-z0-9]){1,4}$",
>   REG_EXTENDED|REG_NOSUB|REG_ICASE,
>   "Please use a more complicated password."
>   },
> - {
> - "^([0-9][a-z]){1,4}$",
> - REG_EXTENDED|REG_NOSUB|REG_ICASE,
> - "Please use a more complicated password."
> - }
>  };
>  
>  int
> 

-- 
Sebastien Marie



ld.lld/ld.bfd difference in -rpath management

2018-10-27 Thread Sebastien Marie
Hi,

I am sending this email on tech@, as I assume it could be interesting
for base developers to be aware of a slightly difference between ld.bfd
and ld.lld regarding -rpath option (and as I sent a previous mail on
ports@ and not everybody follow ports@).

while I investigated a problem on productivity/glabels (see ports@ about
glabels/libtool) I found a difference in rpath management. As it could
be tricky, I prefer to report it to people most aware than me in the
difference.

The short story is:
$ ld.lld -rpath=value   => Add a DT_RUNPATH to the output.
$ ld.bfd -rpath=value   => Add a DT_RPATH to the output.

>From ld.so comments, I think it isn't a problem as it seems just be the
library path search order that could changed a bit.

libexec/ld.so/library_subr.c

 *  Otherwise, the name doesn't contain a '/':
 *  search hints for the specific library version, trying in turn
 *  paths from the following:
 *- the LD_LIBRARY_PATH environment variable (if set)
 *- the library's own DT_RUNPATH
 *- if DT_RUNPATH wasn't set, then:
 *  - the library's own DT_RPATH
 *  - the executable's own DT_RPATH
 *- the default search path set by ldconfig, or /usr/lib if unset

But in case it could be a bit more complex... I prefer to let people
know.

Regarding productivity/glabels, it is us base libtool that doesn't
manage DT_RUNPATH but only DT_RPATH for retrieving library path (a diff
has been provided on ports@).

Thanks.
-- 
Sebastien Marie



Re: unveil xserver's priv proc

2018-10-16 Thread Sebastien Marie
On Tue, Oct 16, 2018 at 12:27:33PM +0100, Ricardo Mestre wrote:
> Hi,
> 
> xserver's priv proc is responsible for opening devices in O_RDWR mode and send
> their fds over to the parent proc. Knowing this then we already have a list of
> all possible devices that might be opened in the future and we can unveil(2)
> them by traversing allowed_devices and yes it's a long list, but we won't hit
> the limit defined by UNVEIL_MAX_VNODES (currently set to 128). But yes, this
> change might be disputable due to a limitation of vnodes available.
> 
> Additionally, by this point we already fork(2)ed so we can drop "proc" promise
> from pledge(2) and I didn't run into any troubles with both these changes.
> 
> Comments on either unveil or pledge? OK?

about unveil: it seems fine. open_ok() functions checks if
cmd.arg.open.path is in allowed_devices. so having it locked to only
that seems correct.


about pledge: "proc" isn't only used for fork(2). but also for using
kill(2) for others pids than itself.

in the main loop, the process could have to send USR1 signal to
parent_pid. if it doesn't have "proc" it will be killed.

   277  if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
   278  err(1, "pledge");
   279
   280  while (1) {
   281  if (read(socks[0], , sizeof(cmd)) == 0) {
   282  exit(0);
   283  }
   284  switch (cmd.cmd) {
   285
   286  case PRIV_OPEN_DEVICE:
   287  if ((dev = open_ok(cmd.arg.open.path)) != NULL) 
{
   288  fd = open(cmd.arg.open.path, 
dev->flags);
   289  } else {
   290  fd = -1;
   291  errno = EPERM;
   292  }
   293  send_fd(socks[0], fd);
   294  if (fd >= 0)
   295  close(fd);
   296  break;
   297  case PRIV_SIG_PARENT:
   298  if (parent_pid > 1)
   299  kill(parent_pid, SIGUSR1);
   300  break;
   301  default:
   302  errx(1, "%s: unknown command %d", __func__, 
cmd.cmd);
   303  break;
   304  }
   305  }
   
> Index: privsep.c
> ===
> RCS file: /cvs/xenocara/xserver/os/privsep.c,v
> retrieving revision 1.29
> diff -u -p -u -r1.29 privsep.c
> --- privsep.c 6 Aug 2018 20:11:34 -   1.29
> +++ privsep.c 16 Oct 2018 10:51:10 -
> @@ -274,7 +274,11 @@ priv_init(uid_t uid, gid_t gid)
>   setproctitle("[priv]");
>   close(socks[1]);
>  
> - if (pledge("stdio rpath wpath sendfd proc", NULL) == -1)
> + for (dev = allowed_devices; dev->name != NULL; dev++) {
> + if (unveil(dev->name, "rw") == -1)
> + err(1, "unveil");
> + }
> + if (pledge("stdio rpath wpath sendfd", NULL) == -1)
>   err(1, "pledge");
>  
>   while (1) {
> 

-- 
Sebastien Marie



savecore: unveil bug

2018-09-28 Thread Sebastien Marie
Hi,

The unveil(2) call for savecore(8) is incomplete. savecore(8) needs to
access to the /bsd to copying it.

Without it, savecore(8) abort the process, and due to karl, the original
kernel is lost.

Without the patch:
--
savecore: reboot after panic: pool_do_get: vmmpepl free list modified: page 
0xff018754; item addr 0xff01875400a8; offset 0x38=0xd6adbeef
savecore: system went down at Fri Sep 28 08:07:31 2018
savecore: /bsd: No such file or directory


I was able to successfully extract the dump with the patch (but /bsd
wasn't the right kernel anymore due to reboot).

Thanks.
-- 
Sebastien Marie


Index: savecore.c
===
RCS file: /cvs/src/sbin/savecore/savecore.c,v
retrieving revision 1.58
diff -u -p -r1.58 savecore.c
--- savecore.c  24 Sep 2018 21:26:38 -  1.58
+++ savecore.c  28 Sep 2018 06:47:58 -
@@ -175,6 +175,10 @@ main(int argc, char *argv[])
syslog(LOG_ERR, "unveil: %m");
exit(1);
}
+   if (unveil(kernel ? kernel : _PATH_UNIX, "r") == -1) {
+   syslog(LOG_ERR, "unveil: %m");
+   exit(1);
+   }
if (pledge("stdio rpath wpath cpath", NULL) == -1) {
syslog(LOG_ERR, "pledge: %m");
exit(1);



Re: yacc + unveil

2018-09-25 Thread Sebastien Marie
On Tue, Sep 25, 2018 at 11:15:43PM +0800, Michael Mikonos wrote:
> On Tue, Sep 25, 2018 at 03:22:38PM +0100, Ricardo Mestre wrote:
> > This is an example of better to start at just hoisting the code that
> > opens the many fds and put them all inside open_files(). After that it's
> > just a matter of calling pledge("stdio") and we are done.
> > 
> > Of course that after this is done we can still make a list of all the files
> > we need to open and unveil them, but not the way it's done here.
> > 
> > Once I get back home from $DAYJOB I'll try to have a look at this.
> 
> After open_files() the wpath pledge can be dropped. rpath is still
> needed because /tmp files are reopened for read in output(). cpath
> is needed because /tmp files are unlinked at the end. This patch
> adds a pledge call, but is it better to just move the first pledge()
> down?
> 

you could try with the "tmppath" promise. I will allow opening/creating
files on /tmp and unlinking them (but not sure it will cover all yacc
need as it is designed for mkstemp(3) family). Unveil for such
operations are fine too, without explicit unveil(2) call.

-- 
Sebastien Marie



Re: c++ headers with latest lib{c++,c++abi,unwind} update / out-of-sync sets

2018-09-14 Thread Sebastien Marie
On Fri, Sep 14, 2018 at 05:19:47PM +1000, Jonathan Gray wrote:
> On Fri, Sep 14, 2018 at 06:18:12AM +0200, Sebastien Marie wrote:
> > Hi,
> > 
> > I noticed the following new headers are installed:
> > 
> > /usr/include/c++/v1/__refstring
> > /usr/include/c++/v1/__undef___deallocate
> > /usr/include/c++/v1/__undef_min_max
> > 
> These headers were removed in the update not added.
> 

ah my bad. I didn't check on an upgraded host if they were used or
not...

sorry for the noise.
-- 
Sebastien Marie



c++ headers with latest lib{c++,c++abi,unwind} update / out-of-sync sets

2018-09-13 Thread Sebastien Marie
Hi,

I noticed the following new headers are installed:

/usr/include/c++/v1/__refstring
/usr/include/c++/v1/__undef___deallocate
/usr/include/c++/v1/__undef_min_max

but they aren't present in sets.

The following diff should resync them.

Thanks.
-- 
Sebastien Marie


Index: clang.amd64
===
RCS file: /cvs/src/distrib/sets/lists/comp/clang.amd64,v
retrieving revision 1.16
diff -u -p -r1.16 clang.amd64
--- clang.amd64 11 Sep 2018 19:43:15 -  1.16
+++ clang.amd64 14 Sep 2018 04:13:00 -
@@ -17,6 +17,7 @@
 ./usr/include/c++/v1/__locale
 ./usr/include/c++/v1/__mutex_base
 ./usr/include/c++/v1/__nullptr
+./usr/include/c++/v1/__refstring
 ./usr/include/c++/v1/__split_buffer
 ./usr/include/c++/v1/__sso_allocator
 ./usr/include/c++/v1/__std_stream
@@ -24,7 +25,9 @@
 ./usr/include/c++/v1/__threading_support
 ./usr/include/c++/v1/__tree
 ./usr/include/c++/v1/__tuple
+./usr/include/c++/v1/__undef___deallocate
 ./usr/include/c++/v1/__undef_macros
+./usr/include/c++/v1/__undef_min_max
 ./usr/include/c++/v1/algorithm
 ./usr/include/c++/v1/any
 ./usr/include/c++/v1/array
Index: clang.arm64
===
RCS file: /cvs/src/distrib/sets/lists/comp/clang.arm64,v
retrieving revision 1.9
diff -u -p -r1.9 clang.arm64
--- clang.arm64 11 Sep 2018 19:43:15 -  1.9
+++ clang.arm64 14 Sep 2018 04:14:28 -
@@ -17,6 +17,7 @@
 ./usr/include/c++/v1/__locale
 ./usr/include/c++/v1/__mutex_base
 ./usr/include/c++/v1/__nullptr
+./usr/include/c++/v1/__refstring
 ./usr/include/c++/v1/__split_buffer
 ./usr/include/c++/v1/__sso_allocator
 ./usr/include/c++/v1/__std_stream
@@ -24,7 +25,9 @@
 ./usr/include/c++/v1/__threading_support
 ./usr/include/c++/v1/__tree
 ./usr/include/c++/v1/__tuple
+./usr/include/c++/v1/__undef___deallocate
 ./usr/include/c++/v1/__undef_macros
+./usr/include/c++/v1/__undef_min_max
 ./usr/include/c++/v1/algorithm
 ./usr/include/c++/v1/any
 ./usr/include/c++/v1/array
Index: clang.armv7
===
RCS file: /cvs/src/distrib/sets/lists/comp/clang.armv7,v
retrieving revision 1.8
diff -u -p -r1.8 clang.armv7
--- clang.armv7 11 Sep 2018 19:43:15 -  1.8
+++ clang.armv7 14 Sep 2018 04:14:50 -
@@ -17,6 +17,7 @@
 ./usr/include/c++/v1/__locale
 ./usr/include/c++/v1/__mutex_base
 ./usr/include/c++/v1/__nullptr
+./usr/include/c++/v1/__refstring
 ./usr/include/c++/v1/__split_buffer
 ./usr/include/c++/v1/__sso_allocator
 ./usr/include/c++/v1/__std_stream
@@ -24,7 +25,9 @@
 ./usr/include/c++/v1/__threading_support
 ./usr/include/c++/v1/__tree
 ./usr/include/c++/v1/__tuple
+./usr/include/c++/v1/__undef___deallocate
 ./usr/include/c++/v1/__undef_macros
+./usr/include/c++/v1/__undef_min_max
 ./usr/include/c++/v1/algorithm
 ./usr/include/c++/v1/any
 ./usr/include/c++/v1/array
Index: clang.i386
===
RCS file: /cvs/src/distrib/sets/lists/comp/clang.i386,v
retrieving revision 1.16
diff -u -p -r1.16 clang.i386
--- clang.i386  11 Sep 2018 19:43:15 -  1.16
+++ clang.i386  14 Sep 2018 04:15:11 -
@@ -17,6 +17,7 @@
 ./usr/include/c++/v1/__locale
 ./usr/include/c++/v1/__mutex_base
 ./usr/include/c++/v1/__nullptr
+./usr/include/c++/v1/__refstring
 ./usr/include/c++/v1/__split_buffer
 ./usr/include/c++/v1/__sso_allocator
 ./usr/include/c++/v1/__std_stream
@@ -24,7 +25,9 @@
 ./usr/include/c++/v1/__threading_support
 ./usr/include/c++/v1/__tree
 ./usr/include/c++/v1/__tuple
+./usr/include/c++/v1/__undef___deallocate
 ./usr/include/c++/v1/__undef_macros
+./usr/include/c++/v1/__undef_min_max
 ./usr/include/c++/v1/algorithm
 ./usr/include/c++/v1/any
 ./usr/include/c++/v1/array
Index: clang.sparc64
===
RCS file: /cvs/src/distrib/sets/lists/comp/clang.sparc64,v
retrieving revision 1.5
diff -u -p -r1.5 clang.sparc64
--- clang.sparc64   11 Sep 2018 19:43:15 -  1.5
+++ clang.sparc64   14 Sep 2018 04:15:30 -
@@ -17,6 +17,7 @@
 ./usr/include/c++/v1/__locale
 ./usr/include/c++/v1/__mutex_base
 ./usr/include/c++/v1/__nullptr
+./usr/include/c++/v1/__refstring
 ./usr/include/c++/v1/__split_buffer
 ./usr/include/c++/v1/__sso_allocator
 ./usr/include/c++/v1/__std_stream
@@ -24,7 +25,9 @@
 ./usr/include/c++/v1/__threading_support
 ./usr/include/c++/v1/__tree
 ./usr/include/c++/v1/__tuple
+./usr/include/c++/v1/__undef___deallocate
 ./usr/include/c++/v1/__undef_macros
+./usr/include/c++/v1/__undef_min_max
 ./usr/include/c++/v1/algorithm
 ./usr/include/c++/v1/any
 ./usr/include/c++/v1/array



Re: add missing break on kern_pledge.c

2018-09-12 Thread Sebastien Marie
On Wed, Sep 12, 2018 at 10:56:19AM +0100, Ricardo Mestre wrote:
> Hi,
> 
> When unveil(2) was introduced one break from SYS_stat case was removed on
> kern_pledge.c, this adds it back. Noticed by Coverity 1471854.
> 
> OK?

the removal was introduced by 1.236 on Jul 13, 2018
- 
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/kern/kern_pledge.c.diff?r1=1.235=1.236
- 
https://github.com/openbsd/src/commit/8b23add8c74b86d0da67de43302cf21b97b028be#diff-7fcc25d727d0f17575db87c6fdc61fafL603

I agree it should be restored : it allows some intented access(2) on
specific system files which where only intented for open(2). it
shouldn't be a big problem, but there is also some checks for activation
yp stuff in this section.

Thanks.
-- 
Sebastien Marie

> Index: kern_pledge.c
> ===
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.242
> diff -u -p -u -r1.242 kern_pledge.c
> --- kern_pledge.c 20 Aug 2018 10:00:04 -  1.242
> +++ kern_pledge.c 12 Sep 2018 09:46:16 -
> @@ -623,6 +623,7 @@ pledge_namei(struct proc *p, struct name
>   } else
>   return (pledge_fail(p, error, PLEDGE_GETPW));
>   }
> + break;
>   case SYS_open:
>   /* daemon(3) or other such functions */
>   if ((ni->ni_pledge & ~(PLEDGE_RPATH | PLEDGE_WPATH)) == 0 &&
> 



Re: drm_wait_one_vblank() fix

2018-09-11 Thread Sebastien Marie
On Mon, Sep 10, 2018 at 08:53:15PM +0200, Mark Kettenis wrote:
> When we're cold, vblank interrupts don't happen, so we would wait
> forever.  I added a short-circuit for this case, but apparently some
> output connector detection code in inteldrm(4) relies on a delay here
> to reliably detect whether something is connected or not.
> 
> I tried fixing this in our Linux compat code, but I didn't come up
> with a working solution.  So this adds an #ifdef __OpenBSD__ block in
> the drm_wait_one_vblank() implementations that we hopefully won't miss
> when updating the drm code in the future.
> 
> Thanks to Philippe Meunier for figuring out the root cause of the
> problem.  And sorry for the delay...
> 
> ok?

Mostly for reporting "it works". So I am fine with it. :)

I tested it on two differents laptops with inteldrm(4) and screen
problems:

- on i386 (Intel 82945GM Video)
console switchs from "848x480, 32bpp" to "1280x800, 32bpp"

- on amd64 (Intel 82945GM Video too)
console switchs from "848x480, 32bpp" to "1024x768, 32bpp" (with 
external VGA attached)

and now I am able to boot with a second screen attached to VGA
without problem. before when connected during boot, the output
was only on VGA and LVDS was black.

Thanks.
-- 
Sebastien Marie

> Index: dev/pci/drm/drm_irq.c
> ===
> RCS file: /cvs/src/sys/dev/pci/drm/drm_irq.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 drm_irq.c
> --- dev/pci/drm/drm_irq.c 28 Mar 2018 05:27:28 -  1.70
> +++ dev/pci/drm/drm_irq.c 10 Sep 2018 18:45:33 -
> @@ -1317,8 +1317,21 @@ void drm_wait_one_vblank(struct drm_devi
>   int ret;
>   u32 last;
>  
> - if (WARN_ON(pipe >= dev->num_crtcs) || cold)
> + if (WARN_ON(pipe >= dev->num_crtcs))
>   return;
> +
> +#ifdef __OpenBSD__
> + /*
> +  * If we're cold, vblank interrupts won't happen even if
> +  * they're turned on by the driver.  Just stall long enough
> +  * for a vblank to pass.  This assumes a vrefresh of at least
> +  * 25 Hz.
> +  */
> + if (cold) {
> + delay(4);
> + return;
> + }
> +#endif
>  
>   ret = drm_vblank_get(dev, pipe);
>   if (WARN(ret, "vblank not available on crtc %i, ret=%i\n", pipe, ret))
> 



Re: xidle: launching program on timeout without active-area

2018-09-03 Thread Sebastien Marie
ping

On Tue, Aug 14, 2018 at 06:15:08AM +0200, Sebastien Marie wrote:
> ping
> 
> On Wed, Jul 25, 2018 at 02:13:49PM +0200, Sebastien Marie wrote:
> > On Wed, Jul 25, 2018 at 12:55:48PM +0200, Claudio Jeker wrote:
> > > On Wed, Jul 25, 2018 at 12:27:29PM +0200, Sebastien Marie wrote:
> > > > On Mon, Jul 16, 2018 at 11:37:41AM +0200, Sebastien Marie wrote:
> > > > 
> > > > > xidle(1) seems great for such purpose. But I didn't found a way to 
> > > > > just
> > > > > use timeout and not also an active area (a corner where the program is
> > > > > launched if pointer stays inside few seconds).
> > > > > 
> > > > > The following diff tries to implement a way to disable the active area
> > > > > without being too intrusive.
> > > > > 
> > > > > For that, I used the `delay' parameter ("Specify the number of seconds
> > > > > the pointer has to be in the given position before running the
> > > > > program."), to allow value -1, and make it to discard the event.
> > > > > 
> > > > > Does it make sens ? Or any proposition to more straighfull approch ?
> > > > > 
> > > 
> > > I would love to be able to use xidle without active area but I have to say
> > > that your approach with -1 as delay seems a bit like a hack. Wouldn't it
> > > be better to add a -no option to disable the corners all together? Maybe
> > > even make that the default instead of -nw?
> > > 
> > 
> > the "-delay -1" approch was taken to avoid too instrusive change that
> > would clash with upstream (but are we upstream ? I didn't found xidle under
> > www.x.org). Anyway, the approch with -no option seems to not be too
> > intrusive neither and it is better for user point of vue.
> > 
> > So below a new diff with -no option.
> > 
> > When used, the -no flag that sets `position' variable to `none'. The
> > active area window is still created (its avoid to manage a new case
> > where `xi->win' could be NULL), but the window isn't mapper and no
> > event asked for EnterWindow.
> > 
> > Sending USR1 still work as intented.
> > 
> > I didn't change the default value for the position, but it could be
> > easily done (one line change in xidle.c and xidle.1).
> > 
> > -- 
> > Sebastien Marie
> > 
> > Index: xidle.1
> > ===
> > RCS file: /cvs/xenocara/app/xidle/xidle.1,v
> > retrieving revision 1.4
> > diff -u -p -r1.4 xidle.1
> > --- xidle.1 9 Nov 2017 19:13:03 -   1.4
> > +++ xidle.1 25 Jul 2018 11:54:13 -
> > @@ -35,7 +35,7 @@
> >  .Op Fl area Ar pixels
> >  .Op Fl delay Ar secs
> >  .Op Fl display Ar display
> > -.Op Fl nw | ne | sw | se
> > +.Op Fl no | nw | ne | sw | se
> >  .Op Fl program Ar path
> >  .Op Fl timeout Ar secs
> >  .Ek
> > @@ -66,8 +66,8 @@ The default is 2 seconds.
> >  .It Fl display Ar display
> >  This argument allows you to specify the server to connect to; see
> >  .Xr X 7 .
> > -.It Fl nw | ne | sw | se
> > -Set the position to one of northwest, northeast, southwest, or southeast,
> > +.It Fl no | nw | ne | sw | se
> > +Set the position to one of none, northwest, northeast, southwest, or 
> > southeast,
> >  respectively.
> >  If no position is specified,
> >  the default is northwest.
> > @@ -100,7 +100,9 @@ Specify the number of seconds to wait be
> >  .Fl delay
> >  option.
> >  .It Sy position No (class Sy Position )
> > -Set the position to one of: "nw", "ne", "sw", or "se"; see descriptions of 
> > the
> > +Set the position to one of: "no", "nw", "ne", "sw", or "se"; see 
> > descriptions
> > +of the
> > +.Fl no ,
> >  .Fl nw ,
> >  .Fl ne ,
> >  .Fl sw ,
> > Index: xidle.c
> > ===
> > RCS file: /cvs/xenocara/app/xidle/xidle.c,v
> > retrieving revision 1.5
> > diff -u -p -r1.5 xidle.c
> > --- xidle.c 20 Aug 2017 16:43:25 -  1.5
> > +++ xidle.c 25 Jul 2018 11:59:40 -
> > @@ -53,7 +53,8 @@ enum {
> > north = 0x01,
> > south = 0x02,
> > east  = 0x04,
> > -   west  = 0x08
> > +   west  = 0x08,
> > +   none  = 0x10,
> >  };
> >  
> >  enum { XIDLE_LOCK = 1, XIDLE_DI

Re: xidle: launching program on timeout without active-area

2018-08-13 Thread Sebastien Marie
ping

On Wed, Jul 25, 2018 at 02:13:49PM +0200, Sebastien Marie wrote:
> On Wed, Jul 25, 2018 at 12:55:48PM +0200, Claudio Jeker wrote:
> > On Wed, Jul 25, 2018 at 12:27:29PM +0200, Sebastien Marie wrote:
> > > On Mon, Jul 16, 2018 at 11:37:41AM +0200, Sebastien Marie wrote:
> > > 
> > > > xidle(1) seems great for such purpose. But I didn't found a way to just
> > > > use timeout and not also an active area (a corner where the program is
> > > > launched if pointer stays inside few seconds).
> > > > 
> > > > The following diff tries to implement a way to disable the active area
> > > > without being too intrusive.
> > > > 
> > > > For that, I used the `delay' parameter ("Specify the number of seconds
> > > > the pointer has to be in the given position before running the
> > > > program."), to allow value -1, and make it to discard the event.
> > > > 
> > > > Does it make sens ? Or any proposition to more straighfull approch ?
> > > > 
> > 
> > I would love to be able to use xidle without active area but I have to say
> > that your approach with -1 as delay seems a bit like a hack. Wouldn't it
> > be better to add a -no option to disable the corners all together? Maybe
> > even make that the default instead of -nw?
> > 
> 
> the "-delay -1" approch was taken to avoid too instrusive change that
> would clash with upstream (but are we upstream ? I didn't found xidle under
> www.x.org). Anyway, the approch with -no option seems to not be too
> intrusive neither and it is better for user point of vue.
> 
> So below a new diff with -no option.
> 
> When used, the -no flag that sets `position' variable to `none'. The
> active area window is still created (its avoid to manage a new case
> where `xi->win' could be NULL), but the window isn't mapper and no
> event asked for EnterWindow.
> 
> Sending USR1 still work as intented.
> 
> I didn't change the default value for the position, but it could be
> easily done (one line change in xidle.c and xidle.1).
> 
> -- 
> Sebastien Marie
> 
> Index: xidle.1
> ===
> RCS file: /cvs/xenocara/app/xidle/xidle.1,v
> retrieving revision 1.4
> diff -u -p -r1.4 xidle.1
> --- xidle.1   9 Nov 2017 19:13:03 -   1.4
> +++ xidle.1   25 Jul 2018 11:54:13 -
> @@ -35,7 +35,7 @@
>  .Op Fl area Ar pixels
>  .Op Fl delay Ar secs
>  .Op Fl display Ar display
> -.Op Fl nw | ne | sw | se
> +.Op Fl no | nw | ne | sw | se
>  .Op Fl program Ar path
>  .Op Fl timeout Ar secs
>  .Ek
> @@ -66,8 +66,8 @@ The default is 2 seconds.
>  .It Fl display Ar display
>  This argument allows you to specify the server to connect to; see
>  .Xr X 7 .
> -.It Fl nw | ne | sw | se
> -Set the position to one of northwest, northeast, southwest, or southeast,
> +.It Fl no | nw | ne | sw | se
> +Set the position to one of none, northwest, northeast, southwest, or 
> southeast,
>  respectively.
>  If no position is specified,
>  the default is northwest.
> @@ -100,7 +100,9 @@ Specify the number of seconds to wait be
>  .Fl delay
>  option.
>  .It Sy position No (class Sy Position )
> -Set the position to one of: "nw", "ne", "sw", or "se"; see descriptions of 
> the
> +Set the position to one of: "no", "nw", "ne", "sw", or "se"; see descriptions
> +of the
> +.Fl no ,
>  .Fl nw ,
>  .Fl ne ,
>  .Fl sw ,
> Index: xidle.c
> ===
> RCS file: /cvs/xenocara/app/xidle/xidle.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 xidle.c
> --- xidle.c   20 Aug 2017 16:43:25 -  1.5
> +++ xidle.c   25 Jul 2018 11:59:40 -
> @@ -53,7 +53,8 @@ enum {
>   north = 0x01,
>   south = 0x02,
>   east  = 0x04,
> - west  = 0x08
> + west  = 0x08,
> + none  = 0x10,
>  };
>  
>  enum { XIDLE_LOCK = 1, XIDLE_DIE = 2 };
> @@ -84,6 +85,7 @@ static XrmOptionDescRec opts[] = {
>   { "-program",   ".program", XrmoptionSepArg,(caddr_t)NULL },
>   { "-timeout",   ".timeout", XrmoptionSepArg,(caddr_t)NULL },
>  
> + { "-no",".position",XrmoptionNoArg, (caddr_t)"no" },
>   { "-ne",".position",XrmoptionNoArg, (caddr_t)"ne" },
>   { "-nw",".position",XrmoptionNoArg, (caddr_t)"nw" 

Re: Nuke PLEDGE_STAT for further pledge/unveil disentaglement.

2018-08-06 Thread Sebastien Marie
gt; -#define PLEDGE_STAT  0x2000ULL   /* XXX this is a stat */
> -#define PLEDGE_STATLIE   0x4000ULL
> -#define PLEDGE_YPACTIVE  0x8000ULL   /* YP use detected and 
> allowed */
> +#define PLEDGE_STATLIE   0x2000ULL
> +#define PLEDGE_YPACTIVE  0x4000ULL   /* YP use detected and 
> allowed */
>  
>  #ifdef PLEDGENAMES
>  static struct {

please don't change PLEDGE_YPACTIVE neither PLEDGE_STATLIE bits. they
starts wih high bit set and go to lower.

-- 
Sebastien Marie



Re: unveil: incomplete unveil_flagmatch semantic

2018-08-05 Thread Sebastien Marie
On Sat, Aug 04, 2018 at 01:16:44PM -0600, Bob Beck wrote:
> > >   if ((error = namei()) != 0)
> > >   return (error);
> > >   fvp = fromnd.ni_vp;
> > > @@ -2945,6 +2973,7 @@ sys_revoke(struct proc *p, void *v, regi
> > >  
> > >   NDINIT(, LOOKUP, FOLLOW, UIO_USERSPACE, SCARG(uap, path), p);
> > >   nd.ni_pledge = PLEDGE_RPATH | PLEDGE_TTY;
> > > + nd.ni_unveil = UNVEIL_READ;
> > 
> > I would put UNVEIL_READ|UNVEIL_WRITE : the invalidation is a kind of
> > modification.
> > 
> 
> Yeah, I was on the fence on that one when I did it. You are reading
> the tty device from the filesystem, but the thing you are invalidating
> is actually an operation on the tty, not anything to do with the
> filesystem itself - but this could go either way..

Reading carefully sys_revoke(), I saw you don't need write access at all
on the device. You just need to own it (or being super user).

Having only UNVEIL_READ make sens too. I am fine with it.

> Theo? I want your opinion here :) 

-- 
Sebastien Marie



Re: unveil: incomplete unveil_flagmatch semantic

2018-08-04 Thread Sebastien Marie
On Sat, Aug 04, 2018 at 10:40:11AM -0600, Bob Beck wrote:
> On Fri, Aug 03, 2018 at 06:31:00AM +0200, Sebastien Marie wrote:
> > On Thu, Aug 02, 2018 at 03:42:03PM +0200, Sebastien Marie wrote:
> > > On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> > > > yeah the latter will be the way to go
> > > > 
> > > 
> 
> > > new diff with direct lookup using an indirection table.
> > > 
> > 
> > new (emergency) version with PLEDGE_CHOWN consideration for unveil(2).
> > 
> > sorry for having missed it.
> >  
> 
> All good because you gave me inspiration, after I ran your diff. 
> 
> I tied unveil to the pledge flags when I first did it because it was
> convenient - I think this thig with chmod (and the awkwardness of
> PLEDGE_STAT, etc. etc.) just shows that this was a decision of
> convienience in the short term that is going to bite us in the long
> term. 
> 
> The lookup table is clever, but is frankly, voodoo :) I don't like
> trying to follow the logic of what maps to what and be concerned
> about what flags are where just for the sake of this, and it
> makes things ugly to read.
> 
> I think I would rather add my own char to the namei structure, 
> and set it appropriately in the same places that pledge does. IMO
> this makes looking at the source code for system calls much clearer
> int the kernel - rather than trying to fathom in your head how a
> combination of pledge flags will turn into unveil. 

it seems to me it is a bit duplicating annotations: annotating syscalls
for pledge, and annotating syscalls for unveil, while pledge has just
more granuality. but I can understand the problem with an additional
level for translating pledge-flags to unveil-flags.

in consequence, you will have to change some bits in man page, as "r"
will not be anymore "corresponding to the pledge(2) promise rpath" :)

> So this is a somewhat "minimal" diff tha puts the flags in 
> namei.h, and checks them as per your change, but rather
> than using a lookup table just expressly sets them
> for each system call appropriately.. it passes regress
> as is. 
> 
> I think after doing this I can probably go in an get rid of
> the awkward PLEDGE_STAT and simplify BYPASS considerably
> as well, but I will do that separately. 
> 
> ok?

some comments inlined.

and one important for starting: you should consider (ni_unveil==0) as
deny by default, instead of allow by default. eventually having an
panic(9) or an assert(9) (but it should be too early for now in case we
want it).

it is important because else if we miss an explicit initialisation (the
struct is zeroed, ni_unveil=0 by default), it will be an allowed
operation by default and we will *not* see the problem. with deny or a
panic(9) we will not miss it: someone will complain.

for a panic(9), it could be done in namei(), eventually just after
pledge_namei() call, to avoid checking the variable too often.


another important thing I just realized: not all filesystem operations
were pledeable, and some haven't ni_pledge because the function was
unreachable while pledged (but pledge_namei() has a panic for such
cases).

Some examples that will need consideration for unveil(2):
- mount(2)
- unmount(2)
- quotactl(2)
- chroot(2)
- getfh(2)
- acct(2)
- coredump()
- loadfirmware() - I think ifconfig(1) could make the kernel loading a
  firmware for some network card

so having ni_unveil separated from ni_pledge could be good to be able to
manage these namei() calls in unveiled programs.


> Index: kern/kern_unveil.c
> ===
> RCS file: /cvs/src/sys/kern/kern_unveil.c,v
> retrieving revision 1.9
> diff -u -p -u -p -r1.9 kern_unveil.c
> --- kern/kern_unveil.c30 Jul 2018 15:16:27 -  1.9
> +++ kern/kern_unveil.c4 Aug 2018 16:13:07 -
> @@ -40,6 +40,11 @@
>  #define UNVEIL_MAX_VNODES128
>  #define UNVEIL_MAX_NAMES 128
>  
> +#define  UNVEIL_READ 0x01
> +#define  UNVEIL_WRITE0x02
> +#define  UNVEIL_CREATE   0x04
> +#define  UNVEIL_EXEC 0x08
> +

the flags are duplicated with namei.h. I think namei.h is the right
place, and there could be removed from here.

>  static inline int
>  unvname_compare(const struct unvname *n1, const struct unvname *n2)
>  {
> @@ -552,32 +558,32 @@ unveil_flagmatch(struct nameidata *ni, u
>   CLR(ni->ni_pledge, PLEDGE_STATLIE);
>   return 1;
>   }
> - if (ni->ni_pledge & PLEDGE_RPATH) {
> - if ((flags & PLEDGE_RPATH) == 0) {
> + if (ni->ni_unveil & UNVEIL_READ) {
> + if ((flags & UNVEIL_READ) == 0) {
>  #ifdef DEBUG_UNVEIL
> 

Re: unveil: incomplete unveil_flagmatch semantic

2018-08-02 Thread Sebastien Marie
On Thu, Aug 02, 2018 at 03:42:03PM +0200, Sebastien Marie wrote:
> On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> > yeah the latter will be the way to go
> > 
> 
> new diff with direct lookup using an indirection table.
> 

new (emergency) version with PLEDGE_CHOWN consideration for unveil(2).

sorry for having missed it.

Thanks.
-- 
Sebastien Marie

Index: sys/pledge.h
===
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.37
diff -u -p -r1.37 pledge.h
--- sys/pledge.h13 Jul 2018 09:25:23 -  1.37
+++ sys/pledge.h3 Aug 2018 04:19:55 -
@@ -23,54 +23,64 @@
 #include 
 
 /*
- * pledge(2) requests
+ * pledge(2) promises
  */
 #define PLEDGE_ALWAYS  0xULL
+
+/* promises used in ni_pledge */
 #define PLEDGE_RPATH   0x0001ULL   /* allow open for read */
 #define PLEDGE_WPATH   0x0002ULL   /* allow open for write */
 #define PLEDGE_CPATH   0x0004ULL   /* allow creat, mkdir, unlink 
etc */
-#define PLEDGE_STDIO   0x0008ULL   /* operate on own pid */
-#define PLEDGE_TMPPATH 0x0010ULL   /* for mk*temp() */
-#define PLEDGE_DNS 0x0020ULL   /* DNS services */
-#define PLEDGE_INET0x0040ULL   /* AF_INET/AF_INET6 sockets */
-#define PLEDGE_FLOCK   0x0080ULL   /* file locking */
-#define PLEDGE_UNIX0x0100ULL   /* AF_UNIX sockets */
-#define PLEDGE_ID  0x0200ULL   /* allow setuid, setgid, etc */
-#define PLEDGE_TAPE0x0400ULL   /* Tape ioctl */
-#define PLEDGE_GETPW   0x0800ULL   /* YP enables if ypbind.lock */
-#define PLEDGE_PROC0x1000ULL   /* fork, waitpid, etc */
-#define PLEDGE_SETTIME 0x2000ULL   /* able to set/adj time/freq */
-#define PLEDGE_FATTR   0x4000ULL   /* allow explicit file st_* 
mods */
-#define PLEDGE_PROTEXEC0x8000ULL   /* allow use of 
PROT_EXEC */
-#define PLEDGE_TTY 0x0001ULL   /* tty setting */
-#define PLEDGE_SENDFD  0x0002ULL   /* AF_UNIX CMSG fd sending */
-#define PLEDGE_RECVFD  0x0004ULL   /* AF_UNIX CMSG fd receiving */
-#define PLEDGE_EXEC0x0008ULL   /* execve, child is free of 
pledge */
-#define PLEDGE_ROUTE   0x0010ULL   /* routing lookups */
-#define PLEDGE_MCAST   0x0020ULL   /* multicast joins */
-#define PLEDGE_VMINFO  0x0040ULL   /* vminfo listings */
-#define PLEDGE_PS  0x0080ULL   /* ps listings */
-#define PLEDGE_DISKLABEL 0x0200ULL /* disklabels */
-#define PLEDGE_PF  0x0400ULL   /* pf ioctls */
-#define PLEDGE_AUDIO   0x0800ULL   /* audio ioctls */
-#define PLEDGE_DPATH   0x1000ULL   /* mknod & mkfifo */
-#define PLEDGE_DRM 0x2000ULL   /* drm ioctls */
-#define PLEDGE_VMM 0x4000ULL   /* vmm ioctls */
-#define PLEDGE_CHOWN   0x8000ULL   /* chown(2) family */
-#define PLEDGE_CHOWNUID0x0001ULL   /* allow owner/group 
changes */
-#define PLEDGE_BPF 0x0002ULL   /* bpf ioctl */
-#define PLEDGE_ERROR   0x0004ULL   /* ENOSYS instead of kill */
-#define PLEDGE_WROUTE  0x0008ULL   /* interface address ioctls */
-#define PLEDGE_UNVEIL  0x0010ULL   /* allow unveil() */
+#define PLEDGE_DPATH   0x0008ULL   /* mknod & mkfifo */
+#define PLEDGE_FATTR   0x0010ULL   /* allow explicit file st_* 
mods */
+#define PLEDGE_TTY 0x0020ULL   /* tty setting */
+#define PLEDGE_UNIX0x0040ULL   /* AF_UNIX sockets */
+#define PLEDGE_EXEC0x0080ULL   /* execve, child is free of 
pledge */
+#define PLEDGE_CHOWN   0x0100ULL   /* chown(2) family */
+
+#define UNVEIL_PLDGMASK0x01ffULL
+
+/* these could occurs in ni_pledge but are ignored */
+#define PLEDGE_UNVEIL  0x0200ULL   /* allow unveil() */
+#define PLEDGE_STAT0x0400ULL   /* XXX this is a stat */
+#define PLEDGE_STATLIE 0x0800ULL
+
+#define UNVEIL_PLDGIGN (PLEDGE_UNVEIL|PLEDGE_STAT|PLEDGE_STATLIE)
+
+/* others promises */
+#define PLEDGE_STDIO   0x1000ULL   /* operate on own pid */
+#define PLEDGE_TMPPATH 0x2000ULL   /* for mk*temp() */
+#define PLEDGE_DNS 0x4000ULL   /* DNS services */
+#define PLEDGE_INET0x8000ULL   /* AF_INET/AF_INET6 sockets */
+#define PLEDGE_FLOCK   0x0001ULL   /* file locking */
+#define PLEDGE_ID  0x0002ULL   /* allow setuid, setgid, etc */
+#define PLEDGE_TAPE0x0004ULL   /* Tape ioctl */
+#define PLEDGE_GETPW   0x0008ULL   /* YP enables if ypbind.lock */
+#define PLEDGE_PROC0x0010ULL   /* fork, waitpid

Re: panic: unveil_nipledge_lookup: unexpected pledge bits: 8589934592 after update to recent -current

2018-08-02 Thread Sebastien Marie
On Fri, Aug 03, 2018 at 12:02:17AM +0200, Felix Maschek wrote:
> Hi,
> 
> I've updated to the recent -current on my Lenovo T450 and get a kernel panic

> panic: unveil_nipledge_lookup: unexpected pledge bits: 8589934592

this panic is from my diff I recently sent to tech@. it seems it has
been put in snapshots. it also means I was wrong in some place, and that
I didn't check enough it before sending the diff. I am really sorry
about that.

The bit I missed is: 8589934592 => 0x2 which is PLEDGE_CHOWN. It
means all syscalls using this pledge annotation could generate the
panic if the program is used with unveil(2)... bad.

> Any idea how to bring up my system again?

wait for a new snapshot or rebuild a kernel from source (possibly from
another system, as using chown(2) with unveil(2) will generate the
panic).

Sorry.
-- 
Sebastien Marie



Re: unveil: incomplete unveil_flagmatch semantic

2018-08-02 Thread Sebastien Marie
On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> yeah the latter will be the way to go
> 

new diff with direct lookup using an indirection table.

first reorders PLEDGE flags to have:
  - PLEDGE promises that could occurs in ni_pledge and are used for
unveil(2)

  - PLEDGE promises that could occurs in ni_pledge and aren't used for
unveil(2) (as PLEDGE_UNVEIL, PLEDGE_STAT, PLEDGE_STATLIE)

  - others promises

  - so I redefined PLEDGE_USERSET a bit differently (as some of these
flags are used in ni_pledge)

there is 8 different promises for the first category. So we just need a
256 long array to have a table to direct lookup PLEDGE -> UNVEIL.

for representing UNVEIL flags, I changed type from uint64_t to u_char.

I hooked a init function in taskq_init() in order to properly initialize
the lookup array, but I am unsure of the placement.

some names for constants/variable could have better names, but I had no
inspiration.

Thanks.
-- 
Sebastien Marie

Index: sys/pledge.h
===
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.37
diff -u -p -r1.37 pledge.h
--- sys/pledge.h13 Jul 2018 09:25:23 -  1.37
+++ sys/pledge.h2 Aug 2018 12:51:28 -
@@ -23,54 +23,64 @@
 #include 
 
 /*
- * pledge(2) requests
+ * pledge(2) promises
  */
 #define PLEDGE_ALWAYS  0xULL
+
+/* promises used in ni_pledge */
 #define PLEDGE_RPATH   0x0001ULL   /* allow open for read */
 #define PLEDGE_WPATH   0x0002ULL   /* allow open for write */
 #define PLEDGE_CPATH   0x0004ULL   /* allow creat, mkdir, unlink 
etc */
-#define PLEDGE_STDIO   0x0008ULL   /* operate on own pid */
-#define PLEDGE_TMPPATH 0x0010ULL   /* for mk*temp() */
-#define PLEDGE_DNS 0x0020ULL   /* DNS services */
-#define PLEDGE_INET0x0040ULL   /* AF_INET/AF_INET6 sockets */
-#define PLEDGE_FLOCK   0x0080ULL   /* file locking */
-#define PLEDGE_UNIX0x0100ULL   /* AF_UNIX sockets */
-#define PLEDGE_ID  0x0200ULL   /* allow setuid, setgid, etc */
-#define PLEDGE_TAPE0x0400ULL   /* Tape ioctl */
-#define PLEDGE_GETPW   0x0800ULL   /* YP enables if ypbind.lock */
-#define PLEDGE_PROC0x1000ULL   /* fork, waitpid, etc */
-#define PLEDGE_SETTIME 0x2000ULL   /* able to set/adj time/freq */
-#define PLEDGE_FATTR   0x4000ULL   /* allow explicit file st_* 
mods */
-#define PLEDGE_PROTEXEC0x8000ULL   /* allow use of 
PROT_EXEC */
-#define PLEDGE_TTY 0x0001ULL   /* tty setting */
-#define PLEDGE_SENDFD  0x0002ULL   /* AF_UNIX CMSG fd sending */
-#define PLEDGE_RECVFD  0x0004ULL   /* AF_UNIX CMSG fd receiving */
-#define PLEDGE_EXEC0x0008ULL   /* execve, child is free of 
pledge */
-#define PLEDGE_ROUTE   0x0010ULL   /* routing lookups */
-#define PLEDGE_MCAST   0x0020ULL   /* multicast joins */
-#define PLEDGE_VMINFO  0x0040ULL   /* vminfo listings */
-#define PLEDGE_PS  0x0080ULL   /* ps listings */
-#define PLEDGE_DISKLABEL 0x0200ULL /* disklabels */
-#define PLEDGE_PF  0x0400ULL   /* pf ioctls */
-#define PLEDGE_AUDIO   0x0800ULL   /* audio ioctls */
-#define PLEDGE_DPATH   0x1000ULL   /* mknod & mkfifo */
-#define PLEDGE_DRM 0x2000ULL   /* drm ioctls */
-#define PLEDGE_VMM 0x4000ULL   /* vmm ioctls */
-#define PLEDGE_CHOWN   0x8000ULL   /* chown(2) family */
-#define PLEDGE_CHOWNUID0x0001ULL   /* allow owner/group 
changes */
-#define PLEDGE_BPF 0x0002ULL   /* bpf ioctl */
-#define PLEDGE_ERROR   0x0004ULL   /* ENOSYS instead of kill */
-#define PLEDGE_WROUTE  0x0008ULL   /* interface address ioctls */
-#define PLEDGE_UNVEIL  0x0010ULL   /* allow unveil() */
+#define PLEDGE_DPATH   0x0008ULL   /* mknod & mkfifo */
+#define PLEDGE_FATTR   0x0010ULL   /* allow explicit file st_* 
mods */
+#define PLEDGE_TTY 0x0020ULL   /* tty setting */
+#define PLEDGE_UNIX0x0040ULL   /* AF_UNIX sockets */
+#define PLEDGE_EXEC0x0080ULL   /* execve, child is free of 
pledge */
+
+#define UNVEIL_PLDGMASK0x00ffULL
+
+/* these could occurs in ni_pledge but are ignored */
+#define PLEDGE_UNVEIL  0x0100ULL   /* allow unveil() */
+#define PLEDGE_STAT0x0200ULL   /* XXX this is a stat */
+#define PLEDGE_STATLIE 0x0400ULL
+
+#define UNVEIL_PLDGIGN 0x0700ULL
+
+/* others promises */
+#define PLEDGE_STDIO   0x0800ULL   /* operate on own pid */
+#define PLEDGE_TMPPATH 0x1000ULL   /* for mk*temp() */
+#

Re: fix segfault on radiusd(8)

2018-08-01 Thread Sebastien Marie
On Wed, Aug 01, 2018 at 12:54:49PM +0100, Ricardo Mestre wrote:
> Hi,
> 
> When radiusd(8) starts shutting down one of the actions is to iterate through
> the configured modules and freezero(3) each module->secret. By using the 
> config
> from /etc/examples/radiusd.conf which has module bsdauth and radius configured
> and running it with -d then hit ^C it will segfault. This is due to bsdauth
> module not having a secret and therefore when calling strlen(NULL) it just
> segfaults since this is not a valid argument.
> 
> In order to avoid this issue we just have to swap strlen(3) with sizeof().
> 
> OK?

if I didn't mess, module is a `struct radiusd_module', and `secret'
member is defined as `char *'. I expect sizeof(module->secret) to be
always 8 (on amd64): the size of the pointer.

as the variable is dynamically allocated, you should really use strlen()
and checking for NULL before zeroing it.

-- 
Sebastien Marie

> Index: radiusd.c
> ===
> RCS file: /cvs/src/usr.sbin/radiusd/radiusd.c,v
> retrieving revision 1.20
> diff -u -p -u -r1.20 radiusd.c
> --- radiusd.c 13 Jun 2017 05:40:22 -  1.20
> +++ radiusd.c 1 Aug 2018 11:46:43 -
> @@ -1066,7 +1066,7 @@ radiusd_module_stop(struct radiusd_modul
>  {
>   module->stopped = true;
>  
> - freezero(module->secret, strlen(module->secret));
> + freezero(module->secret, sizeof(module->secret));
>   module->secret = NULL;
>  
>   if (module->fd >= 0) {
> 



bsd.port.mk: make clean=build && make : failed

2018-08-01 Thread Sebastien Marie
Hi,

After issuing a "make clean=build" on a port, I am unable to build
again.

"make" failed in 'configure' stage:

$ make
...
/bin/sh: cannot create 
/home/semarie/repos/openbsd/ports/pobj/rust-1.28.0/bin/aclocal: Permission 
denied
*** Error 1 in . 
(/home/semarie/repos/openbsd/ports/infrastructure/mk/bsd.port.mk:2689 
'_post-configure-finalize': @printf '#!/bin/sh\nexit ...)
*** Error 1 in . 
(/home/semarie/repos/openbsd/ports/infrastructure/mk/bsd.port.mk:2708 
'/home/semarie/repos/openbsd/ports/pobj/rust-1.28.0/build-amd64/.configure_done')
*** Error 1 in /home/semarie/ports/mystuff/lang/rust 
(/home/semarie/repos/openbsd/ports/infrastructure/mk/bsd.port.mk:2396 'all')

It is due to _post-configure-finalize target that chmod 555 on the file,
and after that, ksh is unable to override it.

One possibility is to have just 755 on mode.

Another would be to rm -f ${WRKDIR}/bin/${_wrap} before the printf.

Thanks.
-- 
Sebastien Marie


Index: /usr/ports/infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1434
diff -u -p -r1.1434 bsd.port.mk
--- /usr/ports/infrastructure/mk/bsd.port.mk30 Jul 2018 17:03:51 -  
1.1434
+++ /usr/ports/infrastructure/mk/bsd.port.mk1 Aug 2018 08:51:48 -
@@ -2687,7 +2687,7 @@ do-configure:
 _post-configure-finalize:
 .for _wrap in aclocal
@printf '#!/bin/sh\nexit 1\n' > ${WRKDIR}/bin/${_wrap}
-   @chmod 555 ${WRKDIR}/bin/${_wrap}
+   @chmod 755 ${WRKDIR}/bin/${_wrap}
 .endfor
 
 ${_CONFIGURE_COOKIE}: ${_PATCH_COOKIE}



Re: unveil: incomplete unveil_flagmatch semantic

2018-07-31 Thread Sebastien Marie
On Mon, Jul 30, 2018 at 12:00:59PM -0600, Theo de Raadt wrote:
> +   for (i=0; flags[i].pledge != 0; i++)
> +   if (ISSET(pledge_flags, flags[i].pledge)) {
> +   SET(permissions, flags[i].unveil);
> +   CLR(pledge_flags, flags[i].pledge);
> +   }
> 
> Rather than iterating, can this be done as a direct lookup?
> 
> table[PLEDGE_RPATH] = ...
> table[PLEDGE_RPATH | PLEDGE_WPATH] = ..
> 
> unveil = table[pledge & range_enforcing_mask];

direct lookup isn't really possible: we would have to map a 64bits long
array in order to be exhaustive (and catch missing bits).



so I tried several alternate things.

(a) the first one was to try to reduce the array size for iterating, and
collapsing bits.

without detailing, it seems the compiler to be smart enough to unroll
the loops and use constants instead of an indirection with an array
(tested clang on i386, but objdump seems to not show me all the asm, so
I am still a bit unsure). having the "missing bits" mecanism in
DIAGNOSTIC could help too to avoid developpment checks in mainline.

but it has the drawback to put too much hope in the fact the compiler
will do the right thing, specially on all platforms (and compiler
version) we support.



(b) another point could be to extend `struct nameidata' to compute
`ni_unveil' from `ni_pledge' at namei() time. this way, we still derive
unveil flags from pledge flags, but the evaluation is done once per
namei() call (instead of once per compoment).

one possible drawback (or restriction) is the conversion is done on the
*initial* value of ni_pledge. for now, only unveil(2) itself add some
flags to ni_pledge (for example PLEDGE_STATLIE), so it shouldn't be a
problem.



(c) last possibility would be to heavy use macros to deduce UNVEIL
semantic from PLEDGE flags.

it could be done this way:

+#ifdef _KERNEL
+
+/* unveil flags definition (for affectation only) */
+#defineUNVEIL_READ PLEDGE_RPATH
+#defineUNVEIL_WRITEPLEDGE_WPATH
+#defineUNVEIL_CREATE   PLEDGE_CPATH
+#defineUNVEIL_EXEC PLEDGE_EXEC
+
+#definePLEDGE_UVR_MASK (PLEDGE_RPATH|PLEDGE_UNIX)
+#definePLEDGE_UVW_MASK 
(PLEDGE_WPATH|PLEDGE_FATTR|PLEDGE_CHOWN|PLEDGE_TTY|PLEDGE_UNIX)
+#definePLEDGE_UVC_MASK (PLEDGE_CPATH|PLEDGE_DPATH)
+#definePLEDGE_UVX_MASK (PLEDGE_EXEC)
+#definePLEDGE_UVz_MASK (PLEDGE_STAT|PLEDGE_STATLIE)
+
+#definePLEDGE_UV_MASK  
(PLEDGE_UVR_MASK|PLEDGE_UVW_MASK|PLEDGE_UVC_MASK|PLEDGE_UVX_MASK\
+|PLEDGE_UVz_MASK)
+
+/* unveil macros (for manipulating flags) */
+#defineunveilable_flags(f) (((f) & ~(PLEDGE_UV_MASK)) == 0)
+#defineunveil_read_isset(f)((f) & PLEDGE_UVR_MASK)
+#defineunveil_write_isset(f)   ((f) & PLEDGE_UVW_MASK)
+#defineunveil_create_isset(f)  ((f) & PLEDGE_UVC_MASK)
+#defineunveil_exec_isset(f)((f) & PLEDGE_UVX_MASK)
+
+#endif

and later, use only macros for doing checks:

// affectation using UNVEIL_*
case 'r':
*perms |= UNVEIL_READ;
break;

// checking if value is bounded in what unveil expects
if (! unveilable_flags(ni_pledge))
panic("missing bits in pledge->unveil conversion");

// getting unveil semantic from PLEDGE flags
if (unveil_read_isset(ni_pledge))
...


the main drawback I see with this method, is it could be somehow fragile
for developers. for example, if someone doesn't use unveil_write_isset()
macro but prefer to directly use UNVEIL_WRITE for checking "w", he will
be wrong. it seems to me it isn't a clean way to do the thing (even if
it could be efficient).


in resume: it seems to me that doing the conversion in
unveil_flagmatch() is wrong to me. placing it in namei() seems more
consistent (and less resource consuming). it also permits to have simple
interface with UNVEIL constants instead of using macros.

the diff belows implements it (but I also have full diff for others
possibilities).

in particular:
- use separate namespace for UNVEIL and PLEDGE flags
- UNVEIL flags are `int' values (instead of uint64_t)
- add `ni_unveil' member to `struct nameidata'
- complete `ni_unveil' from `ni_pledge' in namei()

-- 
Sebastien Marie

Index: sys/namei.h
===
RCS file: /cvs/src/sys/sys/namei.h,v
retrieving revision 1.35
diff -u -p -r1.35 namei.h
--- sys/namei.h 13 Jul 2018 09:25:23 -  1.35
+++ sys/namei.h 31 Jul 2018 08:49:59 -
@@ -59,6 +59,7 @@ struct nameidata {
struct  vnode *ni_startdir; /* starting directory */
struct  vnode *ni_rootdir;  /* logical root directory */
uint64_t ni_pledge; /* expected pledge for namei */
+   int ni_unveil;  /* derived unveil flags from ni_pl

Re: unveil: incomplete unveil_flagmatch semantic

2018-07-30 Thread Sebastien Marie
On Mon, Jul 30, 2018 at 07:55:35AM -0600, Bob Beck wrote:
> yeah the latter will be the way to go
>

here it is.

Some notes:
- I changed flags definition from uint64_t to int
- I defined `static inline' the function that do the conversion from
  pledge to unveil: having a function is more readable, but it is called
  often (for checking each compoment)

thanks.
-- 
Sebastien Marie

Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.254
diff -u -p -r1.254 proc.h
--- sys/proc.h  28 Jul 2018 18:07:26 -  1.254
+++ sys/proc.h  30 Jul 2018 15:27:22 -
@@ -130,7 +130,7 @@ struct tusage {
 struct unvname {
char*un_name;
size_t  un_namesize;
-   uint64_tun_flags;
+   int un_flags;
RBT_ENTRY(unvnmae)  un_rbt;
 };
 
@@ -424,7 +424,7 @@ struct unveil {
struct vnode*uv_vp;
struct unvname_rbt  uv_names;
struct rwlock   uv_lock;
-   u_int64_t   uv_flags;
+   int uv_flags;
 };
 
 struct uidinfo {
Index: kern/kern_unveil.c
===
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.9
diff -u -p -r1.9 kern_unveil.c
--- kern/kern_unveil.c  30 Jul 2018 15:16:27 -  1.9
+++ kern/kern_unveil.c  30 Jul 2018 17:40:07 -
@@ -40,6 +40,11 @@
 #define UNVEIL_MAX_VNODES  128
 #define UNVEIL_MAX_NAMES   128
 
+#defineUNVEIL_READ 0x01
+#defineUNVEIL_WRITE0x02
+#defineUNVEIL_EXEC 0x04
+#defineUNVEIL_CREATE   0x08
+
 static inline int
 unvname_compare(const struct unvname *n1, const struct unvname *n2)
 {
@@ -50,7 +55,7 @@ unvname_compare(const struct unvname *n1
 }
 
 struct unvname *
-unvname_new(const char *name, size_t size, uint64_t flags)
+unvname_new(const char *name, size_t size, int flags)
 {
struct unvname *ret = malloc(sizeof(struct unvname), M_PROC, M_WAITOK);
ret->un_name = malloc(size, M_PROC, M_WAITOK);
@@ -118,7 +123,7 @@ unveil_delete_names(struct unveil *uv)
 }
 
 void
-unveil_add_name(struct unveil *uv, char *name, uint64_t flags)
+unveil_add_name(struct unveil *uv, char *name, int flags)
 {
struct unvname *unvn;
 
@@ -310,7 +315,7 @@ unveil_lookup(struct vnode *vp, struct p
 }
 
 int
-unveil_parsepermissions(const char *permissions, uint64_t *perms)
+unveil_parsepermissions(const char *permissions, int *perms)
 {
size_t i = 0;
char c;
@@ -319,16 +324,16 @@ unveil_parsepermissions(const char *perm
while ((c = permissions[i++]) != '\0') {
switch (c) {
case 'r':
-   *perms |= PLEDGE_RPATH;
+   *perms |= UNVEIL_READ;
break;
case 'w':
-   *perms |= PLEDGE_WPATH;
+   *perms |= UNVEIL_WRITE;
break;
case 'x':
-   *perms |= PLEDGE_EXEC;
+   *perms |= UNVEIL_EXEC;
break;
case 'c':
-   *perms |= PLEDGE_CPATH;
+   *perms |= UNVEIL_CREATE;
break;
default:
return -1;
@@ -338,7 +343,7 @@ unveil_parsepermissions(const char *perm
 }
 
 int
-unveil_setflags(uint64_t *flags, uint64_t nflags)
+unveil_setflags(int *flags, int nflags)
 {
 #if 0
if (((~(*flags)) & nflags) != 0) {
@@ -403,7 +408,7 @@ unveil_add(struct proc *p, struct nameid
struct unveil *uv;
int directory_add;
int ret = EINVAL;
-   u_int64_t flags;
+   int flags;
 
KASSERT(ISSET(ndp->ni_cnd.cn_flags, HASBUF)); /* must have SAVENAME */
 
@@ -525,13 +530,50 @@ unveil_add(struct proc *p, struct nameid
return ret;
 }
 
+static inline int
+unveil_pledgetopermissions(uint64_t pledge_flags)
+{
+   static const struct {
+   uint64_t pledge;
+   int  unveil;
+   } flags[] = {
+   { PLEDGE_RPATH, UNVEIL_READ },
+   { PLEDGE_WPATH, UNVEIL_WRITE },
+   { PLEDGE_EXEC,  UNVEIL_EXEC },
+   { PLEDGE_CPATH, UNVEIL_CREATE },
+   { PLEDGE_CHOWN, UNVEIL_WRITE },
+   { PLEDGE_DPATH, UNVEIL_CREATE },
+   { PLEDGE_FATTR, UNVEIL_WRITE },
+   { PLEDGE_TTY,   UNVEIL_WRITE },
+   { PLEDGE_UNIX,  UNVEIL_READ|UNVEIL_WRITE },
+   { PLEDGE_STAT,  0 },
+   { PLEDGE_STATLIE, 0 },
+   { 0, 0 },
+   };
+   int i;
+   int permissions = 0;
+
+   for (i=0; flags[i].pledge != 0; i++)
+   if (ISSET(pledge_flags, flags[i].pledge)) {
+   SET(permissions,

unveil: incomplete unveil_flagmatch semantic

2018-07-30 Thread Sebastien Marie
Hi,

I think unveil_flagmatch() isn't complete and/or has not the right
semantic.

A bit of internals for starting (I will speak about ni_pledge, people
that know what it is and how it works with pledge/unveil could go to
"what is the problem" part).

unveil(2) works with the syscall annotation introduced for pledge(2).

When kernel needs to resolv a name to vnode, it used namei() function.
For that it initializes a special structure used as namei() argument:
`struct nameidata`. This struct has a field `ni_pledge` used to let vfs
subsystem know what kind of syscalls called it. This way, underline
subsystem doesn't have to know all syscalls, and could work on them by
"group".

For example, when you call open(2), depending the flags argument used,
ni_pledge will contain PLEDGE_RPATH, PLEDGE_WPATH, or PLEDGE_CPATH. The
subsystem has a quick and accurate view of the intented usage of the
vnode.

pledge(2) uses it to check that you have the promise of intent to use,
and unveil(2) uses it too in a similar way, in particular in
unveil_flagmatch() to check if the process has the accurate permission
for this particular vnode.



Now, what is the problem with unveil_flagmatch() ?

If I exclude the PLEDGE_STAT stuff from the equation (I am not still
convinced it is the right way to do it - but it isn't the question for
now), unveil_flagmatch() has a default policy to allow the operation,
and only check for some flags in ni_pledge to deny it.

For simple syscall like open(2) there is no problem. The possible
value of ni_pledge are composed with only PLEDGE_RPATH, PLEDGE_WPATH, or
PLEDGE_CPATH.

But for some others syscalls, it isn't the case.

For example, chmod(2) will set ni_pledge to "PLEDGE_FATTR |
PLEDGE_RPATH". For pledge(2), it means you must have "fattr" (capability
to change attribute on the node) and "rpath" (capability to read the
node) promise to use such syscall.

As unveil(2) doesn't have the "fattr" concept, and unveil_flagmatch()
will check only for PLEDGE_RPATH, PLEDGE_WPATH, PLEDGE_CPATH and
PLEDGE_EXEC, we end with unsound policy: you could use chmod(2) with just
"r" on unveiled part of the filesystem.

Some others flags could occurs in ni_pledge:
- PLEDGE_CHOWN: chown(2) family
- PLEDGE_DPATH: mknod(2), mkfifo(2)
- PLEDGE_FATTR: utimes(2) family, chmod(2) family, truncate(2), chflags(2)
- PLEDGE_TTY:   revoke(2)
- PLEDGE_UNIX:  bind(2), connect(2)

unveil_flagmatch() has a special case for "" flag: it means deny.
but as soon as you have "r", "w", "x" or "c", you have an allow policy
by default. Check will be done only for a part of ni_pledge.


I see basically two possibilities:
- extending unveil(2) permissions to match pledge(2) flags - but I don't
  like it, unveil(2) should be kept simple.

- having a separate namespace for unveil and pledge flags (to avoid
  confusion), and translating all pledge flags to unveil flags.

  PLEDGE_RPATH => UNVEIL_READ
  PLEDGE_WPATH => UNVEIL_WRITE
  PLEDGE_CPATH => UNVEIL_CREATE
  PLEDGE_EXEC  => UNVEIL_EXEC
  PLEDGE_CHOWN => UNVEIL_WRITE
  PLEDGE_DPATH => UNVEIL_CREATE
  PLEDGE_FATTR => UNVEIL_WRITE
  PLEDGE_TTY   => UNVEIL_WRITE
  PLEDGE_UNIX  => UNVEIL_READ|UNVEIL_WRITE (requiring both)
  any others   => panic(9)

This way, we could be really exhaustive in unveil_flagmatch() without
having to bother for future PLEDGE flag addition (as we will panic(9) if
some developer doesn't add it where intented).

Thanks.
-- 
Sebastien Marie



Re: unveil in sndiod

2018-07-30 Thread Sebastien Marie
On Mon, Jul 30, 2018 at 11:26:16AM +0200, Alexandre Ratchov wrote:
> 
> The other sndiod process has neither of rpath, wpath, cpath, or exec,
> so it doesn't need unveil, right?

I am just replying for this aspect of unveil/pledge.

Yes, if the process doesn't have such promises, calling unveil(2) is
unnecessary.

In fact, if you called unveil(2) previously, when you will call
pledge(2), the kernel code will check if you need your unveil
configuration or not, and free it if it isn't the case.
-- 
Sebastien Marie



Re: [bugfix] xterm(1) needs "cpath" pledge(2)

2018-07-29 Thread Sebastien Marie
On Sun, Jul 29, 2018 at 08:43:22AM +0200, Matthieu Herrb wrote:
> On Sun, Jul 29, 2018 at 07:28:19AM +0200, Sebastien Marie wrote:
> > 
> > but to decide, we should know *what* triggered this behaviour.
> 
> Hi,
> 
> After digging a bit, there is at least the 'Print All Immediatly'
> function from button 1 menu that will trigger the creation of a file
> and violate  the pledge.
> 
> see xtermPrintImmediately() in print.c:789. The fopen() itself appears
> in charToPrinter() on line 498 of the same file.

Hi Matthieu and Lauri,

I didn't expected someone else did the homework of Leonid so quickly :)
but thanks for digging in this problem.

> Should this feature be disabled in xterm ?

As xterm has an external upstream, just disabling the feature could be
annoying for future merges. And such feature could be legitimate too. I
would like to hear what others think about it.

The problem is this command doesn't have switch we could use to decide
if we add "cpath" in pledge(2) or not ; as we have already done for
exec-formated or exec-selectable commands and "exec" promise.


I wonder if unveil(2) could help here: unveiling for "cpath" only the
directory where file creation could occurs ? But I need to check some
points on unveil(2) first...

Thanks.
-- 
Sebastien Marie



Re: [bugfix] xterm(1) needs "cpath" pledge(2)

2018-07-28 Thread Sebastien Marie
Hi,

First, thanks to (trying) to search for solve the problem you encountered.

On Sat, Jul 28, 2018 at 10:08:04PM +0300, Leonid Bobrov wrote:
> Hi!
> 
> Like I said yesterday, I don't know how to reproduce this bug, it just
> happened to me and I got this dmesg(1):
> xterm[90461]: pledge "cpath", syscall 5
> 
> Right now I quickly grep(1)'ed xterm(1)'s source code:
> mazocomp$ egrep "mkdir\(|unlink\(|rmdir\(" -R . -n
> ./misc.c:760:   && mkdir(filename, 0700) == 0) {
> ./misc.c:804:   unlink(xterm_cursor_theme);
> ./misc.c:805:   rmdir(my_path);
> 

your search isn't accurate regarding the error message you got.

the pledge error in dmesg could be read as:
xterm (pid 90461) has been killed due to pledge violation.
the program tried to use syscall 5 using "cpath" promise, but it
didn't pledged for it.

what is the syscall 5 ?

$ grep '5$' /usr/include/sys/syscall.h
#define SYS_open5

So it is an open(2) call which need "cpath".

if it needs "cpath", the flags argument of open(2) should contains
O_CREAT:

kern/vfs_syscalls.c
  1018  if (oflags & O_CREAT)
  1019  ni_pledge |= PLEDGE_CPATH;


so at some point after calling pledge(2), xterm called open(?, ?|O_CREAT).

it could be a direct call to open(2), or an indirect call (like calling
fopen(3) with "w").

if you search in xterm code source, you should find several occurrences
of such calls. But like deraadt@ told you, you should also ensure this
call is effectively *after* the pledge call.

as hint, you know it seems to be triggerable from some keyboard
combinaison. enumerating what xterm does by default for such things
could help too to track the problem.

there is a open(2) call somewhere: the pledge violation is proof of
that. but the solution isn't necessary to allow this file creation (by
bindly extending the pledge promises). it could be to disallow the file
creation.

but to decide, we should know *what* triggered this behaviour.

personally, I like to know that xterm is unable to create a file.

thanks.
-- 
Sebastien Marie



unveil: unused ps_uvactive in process

2018-07-28 Thread Sebastien Marie
Hi,

The field ps_uvactive in `struct process` is unused.

Generally, for checking if unveil is active, checks are done on
(ps_uvvcount != 0) or (ps->ps_uvpaths != NULL).

I assume the field is a left over from previous developpment.

The kernel still build fine without it.

Thanks.
-- 
Sebastien Marie


Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.253
diff -u -p -r1.253 proc.h
--- sys/proc.h  20 Jul 2018 21:57:26 -  1.253
+++ sys/proc.h  28 Jul 2018 17:46:01 -
@@ -204,7 +204,6 @@ struct process {
size_t ps_uvvcount; /* count of unveil vnodes held */
size_t ps_uvncount; /* count of unveil names allocated */
int ps_uvshrink;/* do we need to shrink vnode list */
-   int ps_uvactive;/* is unveil active */
int ps_uvdone;  /* no more unveil is permitted */
int ps_uvpcwdgone;  /* need to reevaluate cwd unveil */
 



unveil(2) man page clarification about path type (is a directory or not)

2018-07-28 Thread Sebastien Marie
Hi,

I think some reorganisation in unveil(2) man page could be good for
comprehension.

The diff belows makes the man page follow the structure for paragraphs:

DESCRIPTION
  §1. global description (unveil removes visibility of the entire filesystem...)
  §2. unveil locking
  §3. flags description ("rwxc")

  §4. behavior when argument `path' is a directory (the directory is
  remembered and implications with remove/recreate)

  §5. behavior when argument `path' isn't a directory (the name in
  directory is remembered and implications with remove/recreate)

  §6. what when unveil violation occurs (EACCESS or ENOENT)
  §7. general recommandation and best practice

my diff mostly moves sentences between §4, §5 and §7. §5 is a new
paragraph.

I think it makes more clear how unveil(2) behave regarding the kind of
`path' (is directory or not).

Before, some elements were "hidden" in the last paragraph about "general
recommandation, best practice, and implications for remove/recreate
depending if `path' is a directory or not".


While here, correct a typo in E2BIG error description: "The addition of
path would exceed the per-process limit for unveiled paths." instead of
"for pledged paths".

Thanks.
-- 
Sebastien Marie

Index: /home/semarie/repos/openbsd/src/lib/libc/sys/unveil.2
===
RCS file: /cvs/src/lib/libc/sys/unveil.2,v
retrieving revision 1.5
diff -u -p -r1.5 unveil.2
--- /home/semarie/repos/openbsd/src/lib/libc/sys/unveil.2   27 Jul 2018 
19:14:45 -  1.5
+++ /home/semarie/repos/openbsd/src/lib/libc/sys/unveil.2   28 Jul 2018 
07:04:54 -
@@ -98,6 +98,16 @@ using
 if and only if no more specific matching
 .Fn unveil
 exists at a lower level.
+Directories are remembered at the time of a call to
+.Fn unveil .
+This means that a directory that is removed and recreated after a call to
+.Fn unveil
+will appear to not exist.
+.Pp
+Non directories are remembered by name within their containing directory,
+and so may be created, removed, or re-created after a call to
+.Fn unveil
+and still appear to exist.
 .Pp
 Attempts to access paths not allowed by
 .Nm
@@ -119,16 +129,6 @@ in an application will require lots of s
 of the interfaces called.
 In most cases it is best practice to unveil the directories
 in which an application makes use of files.
-It is important to consider that directory results are remembered at
-the time of a call to
-.Fn unveil .
-This means that a directory that is removed and recreated after a call to
-.Fn unveil
-will appear to not exist.
-Non directories are remembered by name within their containing directory,
-and so may be created, removed, or re-created after a call to
-.Fn unveil
-and still appear to exist.
 .Sh RETURN VALUES
 .Fn unveil
 returns 0 on success or -1 on failure.
@@ -137,7 +137,7 @@ returns 0 on success or -1 on failure.
 .It E2BIG
 The addition of
 .Ar path
-would exceed the per-process limit for pledged paths.
+would exceed the per-process limit for unveiled paths.
 .It ENOENT
 A directory in
 .Ar path



Re: unveil(2) usage in base

2018-07-26 Thread Sebastien Marie
 unveilcommands() would be a better name :)

> +{
> + char *path;
> + char buf[PATH_MAX];
> + char *p, *cp;
> +
> + path = strdup(ipath);
> + if (!path)
> + err(1, "copying path");
> +
> + for (p = path; p && *p;) {
> + cp = strsep(, ":");
> + if (cp) {
> + int r = snprintf(buf, sizeof buf, "%s/%s", cp, cmd);
> + if (r == -1 || r >= sizeof buf)
> + errx(1, "snprintf");
> + if (unveil(buf, "x") == -1)
> + err(1, "unveil");
> + }
> + }
> + free(path);
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -364,6 +388,7 @@ main(int argc, char **argv)
>   authuser(myname, login_style, rule->options & PERSIST);
>   }
>  
> + pledgecommands(safepath, cmd);
>   if (pledge("stdio rpath getpw exec id", NULL) == -1)
>   err(1, "pledge");
>  
> Index: usr.bin/ftp/main.c
> ===
> RCS file: /cvs/src/usr.bin/ftp/main.c,v
> retrieving revision 1.120
> diff -u -p -u -r1.120 main.c
> --- usr.bin/ftp/main.c10 Feb 2018 06:25:16 -  1.120
> +++ usr.bin/ftp/main.c12 Jul 2018 16:18:13 -
> @@ -273,6 +273,7 @@ main(volatile int argc, char *argv[])
>   }
>  
>   ttyout = stdout;
> +// espie: fw_update, ^T not working.  due to no TERM= while in rc script?
>   if (isatty(fileno(ttyout)) && !dumb_terminal && foregroundproc())
>   progress = 1;   /* progress bar on if tty is usable */
>  

unrelated to unveil(2)

> Index: usr.bin/top/top.c
> ===
> RCS file: /cvs/src/usr.bin/top/top.c,v
> retrieving revision 1.89
> diff -u -p -u -r1.89 top.c
> --- usr.bin/top/top.c 15 Mar 2017 04:24:14 -  1.89
> +++ usr.bin/top/top.c 12 Jul 2018 16:18:13 -
> @@ -412,6 +412,8 @@ main(int argc, char *argv[])
>   sigprocmask(SIG_BLOCK, , );
>   if (interactive)
>   init_screen();
> + if (pledge("stdio getpw tty proc ps vminfo", NULL) == -1)
> + err(1, "pledge");   
>   (void) signal(SIGINT, leave);
>   siginterrupt(SIGINT, 1);
>   (void) signal(SIGQUIT, leave);

unrelated to unveil(2)

> Index: usr.sbin/bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.209
> diff -u -p -u -r1.209 bgpctl.c
> --- usr.sbin/bgpctl/bgpctl.c  22 Jul 2018 17:07:53 -  1.209
> +++ usr.sbin/bgpctl/bgpctl.c  25 Jul 2018 17:04:18 -
> @@ -192,7 +192,9 @@ main(int argc, char *argv[])
>   break;
>   }
>  
> - if (pledge("stdio rpath wpath unix", NULL) == -1)
> + if (unveil(sockname, "r") == -1)
> + err(1, "unveil");
> + if (pledge("stdio unix", NULL) == -1)
>   err(1, "pledge");
>  
>   if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)

suspirious change in pledge(2)
- before: "stdio rpath wpath unix"
- after:  "stdio unix"

> Index: usr.sbin/portmap/portmap.c
> ===
> RCS file: /cvs/src/usr.sbin/portmap/portmap.c,v
> retrieving revision 1.48
> diff -u -p -u -r1.48 portmap.c
> --- usr.sbin/portmap/portmap.c14 Oct 2015 13:32:44 -  1.48
> +++ usr.sbin/portmap/portmap.c12 Jul 2018 16:18:13 -
> @@ -609,7 +609,7 @@ callit(struct svc_req *rqstp, SVCXPRT *x
>   return;
>   }
>  
> - if (pledge("stdio rpath inet", NULL) == -1)
> + if (pledge("stdio inet", NULL) == -1)
>   err(1, "pledge");
>  
>   port = pml->pml_map.pm_port;

suspirious change in pledge(2)
- before: "stdio rpath inet"
- after:  "stdio inet"

(and diff unrelated to unveil(2))

> Index: usr.sbin/vmctl/main.c
> ===
> RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
> retrieving revision 1.39
> diff -u -p -u -r1.39 main.c
> --- usr.sbin/vmctl/main.c 12 Jul 2018 14:53:37 -  1.39
> +++ usr.sbin/vmctl/main.c 25 Jul 2018 17:04:20 -
> @@ -158,9 +166,14 @@ parse(int argc, char *argv[])
>   res.action = ctl->action;
>   res.ctl = ctl;
>  
> + if (unveil(SOCKET_NAME, "r") == -1)
> + err(1, "unveil");
> + 
>   if (!ctl->has_pledge) {
>   /* pledge(2) default if command doesn't have its own pledge */
> - if (pledge("stdio rpath exec unix getpw", NULL) == -1)
> + if (unveil(VMCTL_CU, "x") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath exec unix getpw paths", NULL) == -1)
>   err(1, "pledge");

"paths" isn't a know promise name (it should be "unveil")

>   }
>   if (ctl->main(, argc, argv) != 0)

-- 
Sebastien Marie



Re: xidle: launching program on timeout without active-area

2018-07-25 Thread Sebastien Marie
On Wed, Jul 25, 2018 at 12:55:48PM +0200, Claudio Jeker wrote:
> On Wed, Jul 25, 2018 at 12:27:29PM +0200, Sebastien Marie wrote:
> > On Mon, Jul 16, 2018 at 11:37:41AM +0200, Sebastien Marie wrote:
> > 
> > > xidle(1) seems great for such purpose. But I didn't found a way to just
> > > use timeout and not also an active area (a corner where the program is
> > > launched if pointer stays inside few seconds).
> > > 
> > > The following diff tries to implement a way to disable the active area
> > > without being too intrusive.
> > > 
> > > For that, I used the `delay' parameter ("Specify the number of seconds
> > > the pointer has to be in the given position before running the
> > > program."), to allow value -1, and make it to discard the event.
> > > 
> > > Does it make sens ? Or any proposition to more straighfull approch ?
> > > 
> 
> I would love to be able to use xidle without active area but I have to say
> that your approach with -1 as delay seems a bit like a hack. Wouldn't it
> be better to add a -no option to disable the corners all together? Maybe
> even make that the default instead of -nw?
> 

the "-delay -1" approch was taken to avoid too instrusive change that
would clash with upstream (but are we upstream ? I didn't found xidle under
www.x.org). Anyway, the approch with -no option seems to not be too
intrusive neither and it is better for user point of vue.

So below a new diff with -no option.

When used, the -no flag that sets `position' variable to `none'. The
active area window is still created (its avoid to manage a new case
where `xi->win' could be NULL), but the window isn't mapper and no
event asked for EnterWindow.

Sending USR1 still work as intented.

I didn't change the default value for the position, but it could be
easily done (one line change in xidle.c and xidle.1).

-- 
Sebastien Marie

Index: xidle.1
===
RCS file: /cvs/xenocara/app/xidle/xidle.1,v
retrieving revision 1.4
diff -u -p -r1.4 xidle.1
--- xidle.1 9 Nov 2017 19:13:03 -   1.4
+++ xidle.1 25 Jul 2018 11:54:13 -
@@ -35,7 +35,7 @@
 .Op Fl area Ar pixels
 .Op Fl delay Ar secs
 .Op Fl display Ar display
-.Op Fl nw | ne | sw | se
+.Op Fl no | nw | ne | sw | se
 .Op Fl program Ar path
 .Op Fl timeout Ar secs
 .Ek
@@ -66,8 +66,8 @@ The default is 2 seconds.
 .It Fl display Ar display
 This argument allows you to specify the server to connect to; see
 .Xr X 7 .
-.It Fl nw | ne | sw | se
-Set the position to one of northwest, northeast, southwest, or southeast,
+.It Fl no | nw | ne | sw | se
+Set the position to one of none, northwest, northeast, southwest, or southeast,
 respectively.
 If no position is specified,
 the default is northwest.
@@ -100,7 +100,9 @@ Specify the number of seconds to wait be
 .Fl delay
 option.
 .It Sy position No (class Sy Position )
-Set the position to one of: "nw", "ne", "sw", or "se"; see descriptions of the
+Set the position to one of: "no", "nw", "ne", "sw", or "se"; see descriptions
+of the
+.Fl no ,
 .Fl nw ,
 .Fl ne ,
 .Fl sw ,
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.5
diff -u -p -r1.5 xidle.c
--- xidle.c 20 Aug 2017 16:43:25 -  1.5
+++ xidle.c 25 Jul 2018 11:59:40 -
@@ -53,7 +53,8 @@ enum {
north = 0x01,
south = 0x02,
east  = 0x04,
-   west  = 0x08
+   west  = 0x08,
+   none  = 0x10,
 };
 
 enum { XIDLE_LOCK = 1, XIDLE_DIE = 2 };
@@ -84,6 +85,7 @@ static XrmOptionDescRec opts[] = {
{ "-program",   ".program", XrmoptionSepArg,(caddr_t)NULL },
{ "-timeout",   ".timeout", XrmoptionSepArg,(caddr_t)NULL },
 
+   { "-no",".position",XrmoptionNoArg, (caddr_t)"no" },
{ "-ne",".position",XrmoptionNoArg, (caddr_t)"ne" },
{ "-nw",".position",XrmoptionNoArg, (caddr_t)"nw" },
{ "-se",".position",XrmoptionNoArg, (caddr_t)"se" },
@@ -108,7 +110,7 @@ usage()
 {
fprintf(stderr, "Usage:\n%s %s\n", __progname,
"[-area pixels] [-delay secs] [-display host:dpy] "
-   "[-ne | -nw | -se | -sw]\n  [-program path] [-timeout secs]");
+   "[-no | -ne | -nw | -se | -sw]\n  [-program path] [-timeout 
secs]");
exit(1);
 }
 
@@ -133,12 +135,14 @@ init_x(struct xinfo *xi, int position, i
  

Re: xidle: launching program on timeout without active-area

2018-07-25 Thread Sebastien Marie
On Mon, Jul 16, 2018 at 11:37:41AM +0200, Sebastien Marie wrote:
> Hi,
> 
> I am looking at a way to auto-logout some workstation on inactivity.

ping.

no problem if there is no interest for such thing in xidle, I will write
a dedicated app for that.

> xidle(1) seems great for such purpose. But I didn't found a way to just
> use timeout and not also an active area (a corner where the program is
> launched if pointer stays inside few seconds).
> 
> The following diff tries to implement a way to disable the active area
> without being too intrusive.
> 
> For that, I used the `delay' parameter ("Specify the number of seconds
> the pointer has to be in the given position before running the
> program."), to allow value -1, and make it to discard the event.
> 
> Does it make sens ? Or any proposition to more straighfull approch ?
> 
> Thanks.
> -- 
> Sebastien Marie
> 
> 
> Index: xidle.1
> ===
> RCS file: /cvs/xenocara/app/xidle/xidle.1,v
> retrieving revision 1.4
> diff -u -p -r1.4 xidle.1
> --- xidle.1   9 Nov 2017 19:13:03 -   1.4
> +++ xidle.1   16 Jul 2018 09:25:40 -
> @@ -63,6 +63,7 @@ The default is 2 pixels.
>  Specify the number of seconds the pointer has to be in the given position
>  before running the program.
>  The default is 2 seconds.
> +The value -1 makes the activation of program on position to be disabled.
>  .It Fl display Ar display
>  This argument allows you to specify the server to connect to; see
>  .Xr X 7 .
> Index: xidle.c
> ===
> RCS file: /cvs/xenocara/app/xidle/xidle.c,v
> retrieving revision 1.5
> diff -u -p -r1.5 xidle.c
> --- xidle.c   20 Aug 2017 16:43:25 -  1.5
> +++ xidle.c   16 Jul 2018 09:23:18 -
> @@ -303,7 +303,7 @@ fail: errx(1, "illegal value -- %s", (
>   }
>   if (getres(, rdb, "delay", "Delay")) {
>   *delay = strtol((char *)value.addr, , 10);
> - if (*p || *delay < 0)
> + if (*p || *delay < -1)
>   goto fail;
>   }
>   if (getres(, rdb, "position", "Position")) {
> @@ -414,6 +414,9 @@ main(int argc, char **argv)
>   break;
>  
>   case EnterNotify:
> + if (delay == -1)
> + break;
> +
>   sleep(delay);
>  
>   XQueryPointer(x.dpy, x.win, ,

-- 
Sebastien Marie



unveil: return EPERM when locked

2018-07-18 Thread Sebastien Marie
Hi,

The unveil man page stands that unveil will return EPERM when locked:

 EPERM  An attempt to add permission to flags was made, or
path was not accessible, or unveil was called after it
was locked

The lock is sets when unveil(NULL, NULL) is called. The syscall will set
`p->p_p->ps_uvdone=1`, and further call to unveil(2) will be refused.

Currently, the syscall returns EINVAL in such case. So make it return
what the documentation said, as it is the more obvious.

While here, few style correction in return statements.

Thanks.
-- 
Sebastien Marie


Index: kern/vfs_syscalls.c
===
RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.294
diff -u -p -r1.294 vfs_syscalls.c
--- kern/vfs_syscalls.c 13 Jul 2018 09:36:00 -  1.294
+++ kern/vfs_syscalls.c 18 Jul 2018 13:47:12 -
@@ -897,21 +897,21 @@ sys_unveil(struct proc *p, void *v, regi
}
 
if (p->p_p->ps_uvdone != 0)
-   return EINVAL;
+   return (EPERM);
 
error = copyinstr(SCARG(uap, flags), cflags, sizeof(cflags), NULL);
if (error)
-   return(error);
+   return (error);
error = copyinstr(SCARG(uap, path), pathname, sizeof(pathname), 
);
if (error)
-   return(error);
+   return (error);
 
 #ifdef KTRACE
if (KTRPOINT(p, KTR_STRUCT))
ktrstruct(p, "unveil", cflags, strlen(cflags));
 #endif
if (pathlen < 2)
-   return EINVAL;
+   return (EINVAL);
 
/* XXX unveil is disabled but returns sucess for now */
return 0;
@@ -929,7 +929,7 @@ sys_unveil(struct proc *p, void *v, regi
 
/*
 * XXX Any access to the file or directory will allow us to
-* pledge path it
+* unveil it
 */
if ((nd.ni_vp &&
(VOP_ACCESS(nd.ni_vp, VREAD, p->p_ucred, p) == 0 ||



Re: unveil(2) for spamlogd(8)

2018-07-18 Thread Sebastien Marie
On Wed, Jul 18, 2018 at 12:59:12PM +0100, Ricardo Mestre wrote:
> Hi,
> 
> Are there any brave souls out there with unveil(2) enabled already?
> 
> If yes please test this diff for spamlogd(8) which seems to only need rw
> access to the file PATH_SPAMD_DB and nothing else.
> 
> Not asking for OKs yet, but if the code pattern is correct can I start looking
> at other programs?

mostly about the code pattern.

first, I didn't know all arcane of unveil, so I could be wrong at some
point. hearing from beck@ would help too :)

- pledge and unveil

  I think, if possible, you should configure unveil(2) before calling
  pledge(2). This way, you don't have to let the "unveil" promise
  allowed.


- locking unveil

  You should call unveil(NULL, NULL) when all your unveil(2) stuff is
  done: this way, you would lock further unveil addition. But with
  pledge(2) call after, any unveil(2) call would abort the program
  anyway (with no "unveil" promise).

> 
> Index: spamlogd.c
> ===
> RCS file: /cvs/src/libexec/spamlogd/spamlogd.c,v
> retrieving revision 1.27
> diff -u -p -u -r1.27 spamlogd.c
> --- spamlogd.c16 Mar 2016 14:47:04 -  1.27
> +++ spamlogd.c18 Jul 2018 11:46:59 -
> @@ -376,12 +376,15 @@ main(int argc, char **argv)
>   }
>  
>   if (syncsend) {
> - if (pledge("stdio rpath wpath inet flock", NULL) == -1)
> + if (pledge("stdio rpath wpath inet flock unveil", NULL) == -1)
>   err(1, "pledge");
>   } else {
> - if (pledge("stdio rpath wpath flock", NULL) == -1)
> + if (pledge("stdio rpath wpath flock unveil", NULL) == -1)
>   err(1, "pledge");
>   }
> +
> + if (unveil(PATH_SPAMD_DB, "rw") == -1)
> + err(1, "unveil");
>  
>   pcap_loop(hpcap, -1, phandler, NULL);
>  
> 

Thanks.
-- 
Sebastien Marie



Re: call for testing: rad(8) - a rtadvd(8) replacement

2018-07-18 Thread Sebastien Marie
On Wed, Jul 18, 2018 at 08:54:51AM +0200, Florian Obser wrote:
> During g2k18 I commited rad(8).
> 
> The latest amd64 and i386 snapshots should contain it with enough
> features to replace rtadvd(8). If you are using rtadvd(8) I'd
> appreciate if you could switch to rad(8) and report back if any
> features are missing.
> 
> The plan is to unhook rtadvd(8) from the build sooner rather than
> later and to ship 6.4 with rad(8) only.
> 

Hi,

I switched my gateway to use rad instead of rtadvd.

So, some questions :-)

First the topology:
- internet connection on pppoe0
- 2 lan interfaces with ipv6: vlan92 and vlan110

I obtain ipv6 on pppoe0 with DHCPv6-PD.

My upstream send me also router-advertisement on this interface.

tcpdump output:
fe80::2a6f:7fff:fe0e:ae80 > ff02::1: icmp6: router 
advertisement(chlim=64, O, pref=medium, router_ltime=1800, reachable_time=0, 
retrans_time=0)(mtu: mtu=1492)(rdnss: lifetime=400s, 
addr=:::::1, addr=:::::1) [icmp6 cksum ok] 
[class 0xe0] (len 64, hlim 255) 


On the gateway, I use the following rad.conf file:

interface vlan92 {
dns {
resolver ":::XX5c:cabe:19ff:fee2:2ced"
}
}

interface vlan110 {
dns {
resolver ":::XX6e:cabe:19ff:fee2:2ced"
}
}


$ ifconfig vlan92
vlan92: flags=8943 mtu 1500
lladdr c8:be:19:e2:2c:ed
index 16 priority 0 llprio 3
encap: vnetid 92 parent re0
groups: vlan
media: Ethernet autoselect (1000baseT full-duplex)
status: active
inet 192.168.92.2 netmask 0xff00 broadcast 192.168.92.255
inet6 fe80::7f08:c8d1:d9fd:1581%vlan92 prefixlen 64 scopeid 0x10
inet6 :::XX5c:cabe:19ff:fee2:2ced prefixlen 64
inet6 :::XX5c::1 prefixlen 64 pltime 599160 vltime 2586360

For now, it works well. But I see the following in syslog:

Jul 18 10:28:05 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::7f08:c8d1:d9fd:1581 on vlan92 # from itself
Jul 18 10:28:06 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::7f08:c8d1:d9fd:1581 on vlan110# from itself
Jul 18 10:29:24 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::2a6f:7fff:fe0e:ae80 on pppoe0 # not managed interface
Jul 18 10:31:12 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::f280:16b4:9c3b:5f8f on vlan92 
Jul 18 10:32:19 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::2a6f:7fff:fe0e:ae80 on pppoe0 # not managed interface
Jul 18 10:33:33 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::7f08:c8d1:d9fd:1581 on vlan92 # from itself
Jul 18 10:35:28 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::2a6f:7fff:fe0e:ae80 on pppoe0 # not managed interface
Jul 18 10:36:49 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::7f08:c8d1:d9fd:1581 on vlan110# from itself
Jul 18 10:38:04 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::2a6f:7fff:fe0e:ae80 on pppoe0 # not managed interface
Jul 18 10:40:08 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::7f08:c8d1:d9fd:1581 on vlan92 # from itself
Jul 18 10:40:47 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::2a6f:7fff:fe0e:ae80 on pppoe0 # not managed interface
Jul 18 10:41:58 alf rad[86733]: RA or RS with hop limit of 255 from 
fe80::61fd:ac94:2a15:bd0b on vlan92

rad(8) seems to log RA/RS from all interfaces:
- from interface not configured for being managed by itself, like pppoe0
- from interface managed by itself and RA sent by itself (shouldn't it
  know it sent it ?)
- from interface managed by itself and RA/RS sent by someone else

I am unsure about the purpose of this log: it seems to be an
unconditional log on RA/RS reception.

It could have value for RA/RS where it isn't sent by rad(8) itself, and
if it is on some configured interface for rad(8). For others cases, I am
unsure.

Thanks for the clarification.
-- 
Sebastien Marie



unveil: incorrect type flags on unvname_new()

2018-07-16 Thread Sebastien Marie
Hi,

While reviewing unveil(2) code, I found an incorrect type on
unvname_new() function: flags argument should be uint64_t.

It is called by unveil_add_name() which uses uint64_t for flags, and
store the value in struct unvname un_flags member which is uint64_t.

Thanks.
-- 
Sebastien Marie


Index: kern/kern_unveil.c
===
RCS file: /cvs/src/sys/kern/kern_unveil.c,v
retrieving revision 1.2
diff -u -p -r1.2 kern_unveil.c
--- kern/kern_unveil.c  13 Jul 2018 13:47:41 -  1.2
+++ kern/kern_unveil.c  16 Jul 2018 13:08:40 -
@@ -50,7 +50,7 @@ unvname_compare(const struct unvname *n1
 }
 
 struct unvname *
-unvname_new(const char *name, size_t size, int flags)
+unvname_new(const char *name, size_t size, uint64_t flags)
 {
struct unvname *ret = malloc(sizeof(struct unvname), M_PROC, M_WAITOK);
ret->un_name = malloc(size, M_PROC, M_WAITOK);



xidle: launching program on timeout without active-area

2018-07-16 Thread Sebastien Marie
Hi,

I am looking at a way to auto-logout some workstation on inactivity.

xidle(1) seems great for such purpose. But I didn't found a way to just
use timeout and not also an active area (a corner where the program is
launched if pointer stays inside few seconds).

The following diff tries to implement a way to disable the active area
without being too intrusive.

For that, I used the `delay' parameter ("Specify the number of seconds
the pointer has to be in the given position before running the
program."), to allow value -1, and make it to discard the event.

Does it make sens ? Or any proposition to more straighfull approch ?

Thanks.
-- 
Sebastien Marie


Index: xidle.1
===
RCS file: /cvs/xenocara/app/xidle/xidle.1,v
retrieving revision 1.4
diff -u -p -r1.4 xidle.1
--- xidle.1 9 Nov 2017 19:13:03 -   1.4
+++ xidle.1 16 Jul 2018 09:25:40 -
@@ -63,6 +63,7 @@ The default is 2 pixels.
 Specify the number of seconds the pointer has to be in the given position
 before running the program.
 The default is 2 seconds.
+The value -1 makes the activation of program on position to be disabled.
 .It Fl display Ar display
 This argument allows you to specify the server to connect to; see
 .Xr X 7 .
Index: xidle.c
===
RCS file: /cvs/xenocara/app/xidle/xidle.c,v
retrieving revision 1.5
diff -u -p -r1.5 xidle.c
--- xidle.c 20 Aug 2017 16:43:25 -  1.5
+++ xidle.c 16 Jul 2018 09:23:18 -
@@ -303,7 +303,7 @@ fail:   errx(1, "illegal value -- %s", (
}
if (getres(, rdb, "delay", "Delay")) {
*delay = strtol((char *)value.addr, , 10);
-   if (*p || *delay < 0)
+   if (*p || *delay < -1)
goto fail;
}
if (getres(, rdb, "position", "Position")) {
@@ -414,6 +414,9 @@ main(int argc, char **argv)
break;
 
case EnterNotify:
+   if (delay == -1)
+   break;
+
sleep(delay);
 
XQueryPointer(x.dpy, x.win, ,



rad(8): add rad.conf to changelist(5)

2018-07-13 Thread Sebastien Marie
Hi,

As rad(8) is linked in the build, I think it makes sens to add rad.conf
to changelist ?

Thanks.
-- 
Sebastien Marie

Index: changelist
===
RCS file: /cvs/src/etc/changelist,v
retrieving revision 1.120
diff -u -p -r1.120 changelist
--- changelist  29 Apr 2018 11:17:02 -  1.120
+++ changelist  13 Jul 2018 06:16:40 -
@@ -102,6 +102,7 @@
 /etc/rc.local
 /etc/rc.securelevel
 /etc/rc.shutdown
+/etc/rad.conf
 /etc/relayd.conf
 /etc/remote
 /etc/resolv.conf



Re: use-after-free in ieee80211_defrag() [from NetBSD]

2018-06-24 Thread Sebastien Marie
On Thu, Jun 21, 2018 at 07:46:12PM +0200, Sebastien Marie wrote:
> Hi,
> 
> m...@netbsd.org has corrected an use-after-free on NetBSD on similar
> code we have.
> 
>   Fix use-after-free, m_cat can free m.
>   
> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/net80211/ieee80211_input.c.diff?r1=1.111=1.112
> 
> 
> From code reading on us side, I think the same problem is present.
> 
> net80211/ieee80211_input.c
>538  /*
>539   * Handle defragmentation (see 9.5 and Annex C).  We support the 
> concurrent
>540   * reception of fragments of three fragmented MSDUs or MMPDUs.
>541   */
>542  struct mbuf *
>543  ieee80211_defrag(struct ieee80211com *ic, struct mbuf *m, int hdrlen)
>544  {
> ...
>597  /* strip 802.11 header and concatenate fragment */
>598  m_adj(m, hdrlen);
>599  m_cat(df->df_m, m);
>600  df->df_m->m_pkthdr.len += m->m_pkthdr.len;
> 

If I don't mess myself, I think the use-after-free is present, but it
lives in dead code. ieee80211_defrag() function seems not be used
anywhere.

It comes from Feb 8, 2009 in a commit from damien@:

revision 1.109
date: 2009/02/08 15:34:39;  author: damien;  state: Exp;  lines: +94 -1;
initial 802.11 defragmentation bits.
the code will allow the concurrent reception of fragments of three
fragmented MSDUs or MMPDUs as required by the 802.11 standard.


But I fail to find if it was used a day or if it is just dead code since
2009.

Whole commit: 
https://github.com/openbsd/src/commit/0c2a2ba16ccde25b321c6ae91f3f5bcf12f981cf
-- 
Sebastien Marie



use-after-free in ieee80211_defrag() [from NetBSD]

2018-06-21 Thread Sebastien Marie
Hi,

m...@netbsd.org has corrected an use-after-free on NetBSD on similar
code we have.

Fix use-after-free, m_cat can free m.

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/net80211/ieee80211_input.c.diff?r1=1.111=1.112


>From code reading on us side, I think the same problem is present.

net80211/ieee80211_input.c
   538  /*
   539   * Handle defragmentation (see 9.5 and Annex C).  We support the 
concurrent
   540   * reception of fragments of three fragmented MSDUs or MMPDUs.
   541   */
   542  struct mbuf *
   543  ieee80211_defrag(struct ieee80211com *ic, struct mbuf *m, int hdrlen)
   544  {
...
   597  /* strip 802.11 header and concatenate fragment */
   598  m_adj(m, hdrlen);
   599  m_cat(df->df_m, m);
   600  df->df_m->m_pkthdr.len += m->m_pkthdr.len;


the second argument of m_cat() `m' could be freed: m_cat() could call
m_free() on it. so I assume it isn't safe to deference it to get
`m_pkthdr.len`.

Here m_cat() code:

kern/uipc_mbuf.c
   772  /*
   773   * Concatenate mbuf chain n to m.
   774   * n might be copied into m (when n->m_len is small), therefore data 
portion of
   775   * n could be copied into an mbuf of different mbuf type.
   776   * Therefore both chains should be of the same type (e.g. MT_DATA).
   777   * Any m_pkthdr is not updated.
   778   */
   779  void
   780  m_cat(struct mbuf *m, struct mbuf *n)
   781  {
   782  while (m->m_next)
   783  m = m->m_next;
   784  while (n) {
   785  if (M_READONLY(m) || n->m_len > M_TRAILINGSPACE(m)) {
   786  /* just join the two chains */
   787  m->m_next = n;
   788  return;
   789  }
   790  /* splat the data from one into the other */
   791  memcpy(mtod(m, caddr_t) + m->m_len, mtod(n, caddr_t),
   792  n->m_len);
   793  m->m_len += n->m_len;
   794  n = m_free(n);
   795  }
   796  }


the NetBSD diff seems to mix 2 things. I understand the first (saving
`m->m_pkthdr.len' in intermediate variable for adjust
`df->df_m->m_pkthdr.len' later), but I am unsure to the second.

The diff below contains only the mlen part.

I quickly checked other uses of m_cat() inside the kernel, and they
seems corrects.

Thanks.
-- 
Sebastien Marie


Index: net80211/ieee80211_input.c
===
RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
retrieving revision 1.201
diff -u -p -r1.201 ieee80211_input.c
--- net80211/ieee80211_input.c  5 May 2018 06:58:05 -   1.201
+++ net80211/ieee80211_input.c  21 Jun 2018 17:37:41 -
@@ -546,7 +546,7 @@ ieee80211_defrag(struct ieee80211com *ic
struct ieee80211_defrag *df;
u_int16_t rxseq, seq;
u_int8_t frag;
-   int i;
+   int i, mlen;
 
wh = mtod(m, struct ieee80211_frame *);
rxseq = letoh16(*(const u_int16_t *)wh->i_seq);
@@ -596,8 +596,9 @@ ieee80211_defrag(struct ieee80211com *ic
df->df_frag = frag;
/* strip 802.11 header and concatenate fragment */
m_adj(m, hdrlen);
+   mlen = m->m_pkthdr.len;
m_cat(df->df_m, m);
-   df->df_m->m_pkthdr.len += m->m_pkthdr.len;
+   df->df_m->m_pkthdr.len += mlen;
 
if (wh->i_fc[1] & IEEE80211_FC1_MORE_FRAG)
return NULL;/* MSDU or MMPDU not yet complete */



Re: smtpd: make relay to smarthost to verify TLS by default

2018-05-31 Thread Sebastien Marie
On Thu, May 31, 2018 at 10:25:54PM +0200, Eric Faurot wrote:
> 
> Hello.
> 
> This makes sense, indeed.
> 
> Here is a slightly updated diff for your proposal.  It makes the
> documentatino more accurate: the server certificate is always
> verified, the flag is only meant to accept invalid certificates.  It
> also fixes build (apparently the mta.c chunk was incorrect).

the initial diff was to set F_TLS_VERIFY by default, and only remove it
on request. with the new diff, the logic is inversed. it makes no
functional changes, but it means the code logic could be more fragile as
if code path changes (in future developpement or refactoring) invalidate
certificate could be accepted by default.

but it is fine with me (and invalid certificates are logged).

another comment inline.

Thanks
-- 
Sebastien Marie

> Index: mta.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/mta.c,v
> retrieving revision 1.212
> diff -u -p -r1.212 mta.c
> --- mta.c 31 May 2018 11:56:10 -  1.212
> +++ mta.c 31 May 2018 19:56:03 -
> @@ -677,6 +677,9 @@ mta_handle_envelope(struct envelope *evp
>   return;
>   }
>  
> + if (dispatcher->u.remote.tls_noverify == 0)
> + evp->agent.mta.relay.flags |= F_TLS_VERIFY;
> +

I am unsure if the condition should be:

if (smarthost && dispatcher->u.remote.tls_noverify == 0)

it ensures the flag will be set only if smarthost is used.

here, the flag will be added everytime (and removed only with "tls
no-verify" which is usable only with "host example.com").

>   relay = mta_relay(evp);
>   /* ignore if we don't know the limits yet */
>   if (relay->limits &&
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
> retrieving revision 1.206
> diff -u -p -r1.206 parse.y
> --- parse.y   30 May 2018 19:01:58 -  1.206
> +++ parse.y   31 May 2018 19:56:04 -
> @@ -182,7 +182,7 @@ typedef struct {
>  %token   KEY
>  %token   LIMIT LISTEN LMTP LOCAL
>  %token   MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE 
> MAX_DEFERRED MBOX MDA MTA MX
> -%token   NODSN
> +%token   NODSN NOVERIFY
>  %token   ON
>  %token   PKI PORT
>  %token   QUEUE
> @@ -541,6 +541,19 @@ HELO STRING {
>  
>   dispatcher->u.remote.smarthost = strdup(t->t_name);
>  }
> +| TLS NOVERIFY {
> + if (dispatcher->u.remote.smarthost == NULL) {
> + yyerror("tls no-verify may not be specified without host on a 
> dispatcher");
> + YYERROR;
> + }
> +
> + if (dispatcher->u.remote.tls_noverify == 1) {
> + yyerror("tls no-verify already specified for this dispatcher");
> + YYERROR;
> + }
> +
> + dispatcher->u.remote.tls_noverify = 1;
> +}
>  | AUTH tables {
>   struct table   *t = $2;
>  
> @@ -1571,6 +1584,7 @@ lookup(char *s)
>   { "mta",MTA },
>   { "mx", MX },
>   { "no-dsn", NODSN },
> + { "no-verify",  NOVERIFY },
>   { "on", ON },
>   { "pki",PKI },
>   { "port",   PORT },
> Index: smtpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
> retrieving revision 1.183
> diff -u -p -r1.183 smtpd.conf.5
> --- smtpd.conf.5  31 May 2018 13:36:35 -  1.183
> +++ smtpd.conf.5  31 May 2018 19:56:04 -
> @@ -205,6 +205,9 @@ to advertise during the HELO phase.
>  .It Cm host Ar relay-url
>  Do not perform MX lookups but relay messages to the relay host described by
>  .Ar relay-url .
> +If the url uses tls, the certificate will be verified by default.
> +.It Cm tls Ar no-verify
> +Do not require valid certificate for the specified host.
>  .It Cm auth Pf < Ar table Ns >
>  Use the mapping
>  .Ar table
> Index: smtpd.h
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
> retrieving revision 1.545
> diff -u -p -r1.545 smtpd.h
> --- smtpd.h   29 May 2018 21:05:52 -  1.545
> +++ smtpd.h   31 May 2018 19:56:05 -
> @@ -1068,6 +1068,7 @@ struct dispatcher_remote {
>  
>   char*smarthost;
>   char*auth;
> + int  tls_noverify;
>  
>   int  backup;
>   char*backupmx;



smtpd: make relay to smarthost to verify TLS by default

2018-05-31 Thread Sebastien Marie
Hi,

When using smarthost ("host" option of "relay") for outgoing mails, TLS
connection aren't verified. If it could make sens for standard MX, I
think it would be better to verify the connection by default if the user
specifies a TLS-aware url for the relay.

The diff below changes the behaviour of:
action "foo" relay host "smtps://example.com"

Currently, smtpd will connect to example.com without verifying TLS at
all. There is no option to force such verification (it was present in
with previous grammar).

With the following diff, the TLS connection is verified by default (and
the connection aborted on error). Opportunistic TLS will be still possible
with a new option: tls no-verify.

So, for having the old behaviour the syntax will be:
action "foo" relay host "smtps://example.com" tls no-verify

It affects only smarthost connection. Outgoing using MX still use
opportunistic TLS.

Regarding the diff, it adds F_TLS_VERIFY option by default for each call
of text_to_relayhost(), so also for "smtp://" or "lmtp://" urls. I checked
that "smtp://" isn't affected by such flag (there is no TLS connection
to verify), and I hope it will be the same for "lmtp://" (confirmation
will be welcome). Next, the grammar is extended to permit to clear the
flag if requested. smtpd(1) already have all the magic to check the
connection.

Thanks.
-- 
Sebastien Marie


Index: smtpd.conf.5
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.183
diff -u -p -r1.183 smtpd.conf.5
--- smtpd.conf.531 May 2018 13:36:35 -  1.183
+++ smtpd.conf.531 May 2018 13:50:55 -
@@ -205,6 +205,9 @@ to advertise during the HELO phase.
 .It Cm host Ar relay-url
 Do not perform MX lookups but relay messages to the relay host described by
 .Ar relay-url .
+If the url uses tls, the connection will be verified by default.
+.It Cm tls Ar no-verify
+Do not perform tls verification for the specified host.
 .It Cm auth Pf < Ar table Ns >
 Use the mapping
 .Ar table
Index: smtpd.h
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.h,v
retrieving revision 1.545
diff -u -p -r1.545 smtpd.h
--- smtpd.h 29 May 2018 21:05:52 -  1.545
+++ smtpd.h 31 May 2018 13:50:56 -
@@ -1068,6 +1068,7 @@ struct dispatcher_remote {
 
char*smarthost;
char*auth;
+   int  tls_noverify;
 
int  backup;
char*backupmx;
Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.30
diff -u -p -r1.30 to.c
--- to.c29 May 2018 21:05:52 -  1.30
+++ to.c31 May 2018 13:50:57 -
@@ -348,7 +348,8 @@ text_to_relayhost(struct relayhost *rela
else
p = buffer + strlen(schemas[i].name);
 
-   relay->flags = schemas[i].flags;
+   /* require valid tls by default (if tls is used) */
+   relay->flags = schemas[i].flags | F_TLS_VERIFY;
 
/* need to specify an explicit port for LMTP */
if (relay->flags & F_LMTP)
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/smtpd/parse.y,v
retrieving revision 1.206
diff -u -p -r1.206 parse.y
--- parse.y 30 May 2018 19:01:58 -  1.206
+++ parse.y 31 May 2018 13:50:58 -
@@ -182,7 +182,7 @@ typedef struct {
 %token KEY
 %token LIMIT LISTEN LMTP LOCAL
 %token MAIL_FROM MAILDIR MASK_SRC MASQUERADE MATCH MAX_MESSAGE_SIZE 
MAX_DEFERRED MBOX MDA MTA MX
-%token NODSN
+%token NODSN NOVERIFY
 %token ON
 %token PKI PORT
 %token QUEUE
@@ -541,6 +541,19 @@ HELO STRING {
 
dispatcher->u.remote.smarthost = strdup(t->t_name);
 }
+| TLS NOVERIFY {
+   if (dispatcher->u.remote.smarthost == NULL) {
+   yyerror("tls no-verify may not be specified without host on a 
dispatcher");
+   YYERROR;
+   }
+
+   if (dispatcher->u.remote.tls_noverify == 1) {
+   yyerror("tls no-verify already specified for this dispatcher");
+   YYERROR;
+   }
+
+   dispatcher->u.remote.tls_noverify = 1;
+}
 | AUTH tables {
struct table   *t = $2;
 
@@ -1571,6 +1584,7 @@ lookup(char *s)
{ "mta",MTA },
{ "mx", MX },
{ "no-dsn", NODSN },
+   { "no-verify",  NOVERIFY },
{ "on", ON },
{ "pki",PKI },
{ "port",   PORT },
Index: mta.c
===
RCS file: /cvs/src/usr

corrections in smtpd.conf(5)

2018-05-31 Thread Sebastien Marie
Hi,

The following diff corrects smtpd.conf man page in two ways:

- Replace virtual(5) reference by table(5) as virtual table format is
  documentation in table(5) man page under "Aliasing tables" section.

- Add "auth " documentation. Example at end of the man page uses
  it, so it should be documented.

Thanks.
-- 
Sebastien Marie


Index: smtpd.conf.5
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.conf.5,v
retrieving revision 1.182
diff -u -p -r1.182 smtpd.conf.5
--- smtpd.conf.530 May 2018 13:44:03 -  1.182
+++ smtpd.conf.531 May 2018 13:14:30 -
@@ -179,9 +179,9 @@ option.
 .It Cm virtual Pf < Ar table Ns >
 Use the mapping
 .Ar table
-for
-.Xr virtual 5
-expansion.
+for virtual expansion.
+The aliasing table format is described in
+.Xr table 5 .
 .El
 .Pp
 The relay delivery methods also support additional options:
@@ -205,6 +205,17 @@ to advertise during the HELO phase.
 .It Cm host Ar relay-url
 Do not perform MX lookups but relay messages to the relay host described by
 .Ar relay-url .
+.It Cm auth Pf < Ar table Ns >
+Use the mapping
+.Ar table
+for connecting to
+.Ar relay-url
+using credentials.
+This option is usable only with
+.Cm host
+option.
+The credential table format is described in
+.Xr table 5 .
 .It Cm mail\-from Ar mailaddr
 Use
 .Ar mailaddr



Re: patch: add --localize-hidden option to objcopy

2018-05-20 Thread Sebastien Marie
On Sun, May 20, 2018 at 11:09:48AM +0200, Mark Kettenis wrote:
> > Date: Sun, 20 May 2018 10:26:44 +0200
> > From: Sebastien Marie <sema...@online.fr>
> > 
> > Hi,
> > 
> > While trying to build git HEAD of radare2, I found us objcopy(1) doesn't
> > have the --localize-hidden option.
> > 
> > As the option was proposed in
> > https://sourceware.org/ml/binutils/2006-06/msg00204.html (in 2006), and
> > commited in GPLv2 tree of binutils, I think the patch is suitable for us
> > too.
> > 
> > Diff to add the option below.
> > 
> > Link to the backported commit:
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=d58c2e3acdba2aadf3a47d741236fa02d7bb04ff;hp=22a84b55803f2adae036b553f7ca347ba02744be
> > 
> > Thanks.
> 
> There is a minor tabs vs. spaces issue in the binutils.texi bit.
> Otherwise this is ok kettenis@
> 

My bad. Here a new diff.

Thanks.
-- 
Sebastien Marie

Index: gnu/usr.bin/binutils-2.17/binutils/doc/binutils.texi
===
RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/binutils/doc/binutils.texi,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 binutils.texi
--- gnu/usr.bin/binutils-2.17/binutils/doc/binutils.texi24 Apr 2011 
20:14:40 -  1.1.1.1
+++ gnu/usr.bin/binutils-2.17/binutils/doc/binutils.texi20 May 2018 
10:18:50 -
@@ -952,6 +952,7 @@ objcopy [@option{-F} @var{bfdname}|@opti
 [@option{-N} @var{symbolname}|@option{--strip-symbol=}@var{symbolname}]
 [@option{--strip-unneeded-symbol=}@var{symbolname}]
 [@option{-G} 
@var{symbolname}|@option{--keep-global-symbol=}@var{symbolname}]
+[@option{--localize-hidden}]
 [@option{-L} 
@var{symbolname}|@option{--localize-symbol=}@var{symbolname}]
 [@option{--globalize-symbol=}@var{symbolname}]
 [@option{-W} 
@var{symbolname}|@option{--weaken-symbol=}@var{symbolname}]
@@ -1123,6 +1124,11 @@ by a relocation.  This option may be giv
 Keep only symbol @var{symbolname} global.  Make all other symbols local
 to the file, so that they are not visible externally.  This option may
 be given more than once.
+
+@item --localize-hidden
+In an ELF object, mark all symbols that have hidden or internal visibility
+as local.  This option applies on top of symbol-specific localization options
+such as @option{-L}.
 
 @item -L @var{symbolname}
 @itemx --localize-symbol=@var{symbolname}
Index: gnu/usr.bin/binutils-2.17/binutils/objcopy.c
===
RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/binutils/objcopy.c,v
retrieving revision 1.4
diff -u -p -r1.4 objcopy.c
--- gnu/usr.bin/binutils-2.17/binutils/objcopy.c15 Nov 2015 04:13:17 
-  1.4
+++ gnu/usr.bin/binutils-2.17/binutils/objcopy.c20 May 2018 10:18:52 
-
@@ -190,6 +190,9 @@ static bfd_boolean remove_leading_char =
 /* Whether to permit wildcard in symbol comparison.  */
 static bfd_boolean wildcard = FALSE;
 
+/* True if --localize-hidden is in effect.  */
+static bfd_boolean localize_hidden = FALSE;
+
 /* List of symbols to strip, keep, localize, keep-global, weaken,
or redefine.  */
 static struct symlist *strip_specific_list = NULL;
@@ -240,6 +243,7 @@ enum command_line_switch
 OPTION_STRIP_UNNEEDED_SYMBOL,
 OPTION_STRIP_UNNEEDED_SYMBOLS,
 OPTION_KEEP_SYMBOLS,
+OPTION_LOCALIZE_HIDDEN,
 OPTION_LOCALIZE_SYMBOLS,
 OPTION_GLOBALIZE_SYMBOL,
 OPTION_GLOBALIZE_SYMBOLS,
@@ -328,6 +332,7 @@ static struct option copy_options[] =
   {"keep-global-symbols", required_argument, 0, OPTION_KEEPGLOBAL_SYMBOLS},
   {"keep-symbol", required_argument, 0, 'K'},
   {"keep-symbols", required_argument, 0, OPTION_KEEP_SYMBOLS},
+  {"localize-hidden", no_argument, 0, OPTION_LOCALIZE_HIDDEN},
   {"localize-symbol", required_argument, 0, 'L'},
   {"localize-symbols", required_argument, 0, OPTION_LOCALIZE_SYMBOLS},
   {"no-adjust-warnings", no_argument, 0, OPTION_NO_CHANGE_WARNINGS},
@@ -428,6 +433,7 @@ copy_usage (FILE *stream, int exit_statu
  --only-keep-debug Strip everything but the debug 
information\n\
   -K --keep-symbol   Do not strip symbol \n\
  --keep-file-symbols   Do not strip file symbol(s)\n\
+ --localize-hidden Turn all ELF hidden symbols into locals\n\
   -L --localize-symbol   Force symbol  to be marked as a 
local\n\
  --globalize-symbol  Force symbol  to be marked as a 
global\n\
   -G --keep-global-symbolLocalize all symbols except \n\
@@ -809,6 +815,24 @@ is_strip_section (bfd *abfd ATTRIBUTE_UN
   return FALSE;
 }
 
+/* Return true if SYM is a hidden symbol.  */
+
+static bfd_boolean
+is_hidden_symbol (asymbol *sym)
+{
+  elf_symbol_type *elf_sym;
+
+  elf_sym = elf_sy

patch: add --localize-hidden option to objcopy

2018-05-20 Thread Sebastien Marie
Hi,

While trying to build git HEAD of radare2, I found us objcopy(1) doesn't
have the --localize-hidden option.

As the option was proposed in
https://sourceware.org/ml/binutils/2006-06/msg00204.html (in 2006), and
commited in GPLv2 tree of binutils, I think the patch is suitable for us
too.

Diff to add the option below.

Link to the backported commit:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=d58c2e3acdba2aadf3a47d741236fa02d7bb04ff;hp=22a84b55803f2adae036b553f7ca347ba02744be

Thanks.
-- 
Sebastien Marie

https://sourceware.org/ml/binutils/2006-06/msg00204.html
from Richard Sandiford

This patch adds a --localize-hidden option to objcopy.  As its name
suggests, it converts all global or weak STV_HIDDEN or STV_INTERNAL
symbols into local symbols.  It is equivalent to listing all such
symbols using separate -L options.

Index: objcopy.c
===
RCS file: /cvs/src/gnu/usr.bin/binutils-2.17/binutils/objcopy.c,v
retrieving revision 1.4
diff -u -p -r1.4 objcopy.c
--- objcopy.c   15 Nov 2015 04:13:17 -  1.4
+++ objcopy.c   20 May 2018 08:06:13 -
@@ -190,6 +190,9 @@ static bfd_boolean remove_leading_char =
 /* Whether to permit wildcard in symbol comparison.  */
 static bfd_boolean wildcard = FALSE;
 
+/* True if --localize-hidden is in effect.  */
+static bfd_boolean localize_hidden = FALSE;
+
 /* List of symbols to strip, keep, localize, keep-global, weaken,
or redefine.  */
 static struct symlist *strip_specific_list = NULL;
@@ -240,6 +243,7 @@ enum command_line_switch
 OPTION_STRIP_UNNEEDED_SYMBOL,
 OPTION_STRIP_UNNEEDED_SYMBOLS,
 OPTION_KEEP_SYMBOLS,
+OPTION_LOCALIZE_HIDDEN,
 OPTION_LOCALIZE_SYMBOLS,
 OPTION_GLOBALIZE_SYMBOL,
 OPTION_GLOBALIZE_SYMBOLS,
@@ -328,6 +332,7 @@ static struct option copy_options[] =
   {"keep-global-symbols", required_argument, 0, OPTION_KEEPGLOBAL_SYMBOLS},
   {"keep-symbol", required_argument, 0, 'K'},
   {"keep-symbols", required_argument, 0, OPTION_KEEP_SYMBOLS},
+  {"localize-hidden", no_argument, 0, OPTION_LOCALIZE_HIDDEN},
   {"localize-symbol", required_argument, 0, 'L'},
   {"localize-symbols", required_argument, 0, OPTION_LOCALIZE_SYMBOLS},
   {"no-adjust-warnings", no_argument, 0, OPTION_NO_CHANGE_WARNINGS},
@@ -428,6 +433,7 @@ copy_usage (FILE *stream, int exit_statu
  --only-keep-debug Strip everything but the debug 
information\n\
   -K --keep-symbol   Do not strip symbol \n\
  --keep-file-symbols   Do not strip file symbol(s)\n\
+ --localize-hidden Turn all ELF hidden symbols into locals\n\
   -L --localize-symbol   Force symbol  to be marked as a 
local\n\
  --globalize-symbol  Force symbol  to be marked as a 
global\n\
   -G --keep-global-symbolLocalize all symbols except \n\
@@ -809,6 +815,24 @@ is_strip_section (bfd *abfd ATTRIBUTE_UN
   return FALSE;
 }
 
+/* Return true if SYM is a hidden symbol.  */
+
+static bfd_boolean
+is_hidden_symbol (asymbol *sym)
+{
+  elf_symbol_type *elf_sym;
+
+  elf_sym = elf_symbol_from (sym->the_bfd, sym);
+  if (elf_sym != NULL)
+switch (ELF_ST_VISIBILITY (elf_sym->internal_elf_sym.st_other))
+  {
+  case STV_HIDDEN:
+  case STV_INTERNAL:
+   return TRUE;
+  }
+  return FALSE;
+}
+
 /* Choose which symbol entries to copy; put the result in OSYMS.
We don't copy in place, because that confuses the relocs.
Return the number of symbols to print.  */
@@ -955,7 +979,8 @@ filter_symbols (bfd *abfd, bfd *obfd, as
  && (flags & (BSF_GLOBAL | BSF_WEAK))
  && (is_specified_symbol (name, localize_specific_list)
  || (keepglobal_specific_list != NULL
- && ! is_specified_symbol (name, 
keepglobal_specific_list
+ && ! is_specified_symbol (name, keepglobal_specific_list))
+ || (localize_hidden && is_hidden_symbol (sym
{
  sym->flags &= ~ (BSF_GLOBAL | BSF_WEAK);
  sym->flags |= BSF_LOCAL;
@@ -1532,6 +1557,7 @@ copy_object (bfd *ibfd, bfd *obfd)
   || strip_symbols == STRIP_UNNEEDED
   || strip_symbols == STRIP_NONDEBUG
   || discard_locals != LOCALS_UNDEF
+  || localize_hidden
   || strip_specific_list != NULL
   || keep_specific_list != NULL
   || localize_specific_list != NULL
@@ -3059,6 +3085,10 @@ copy_main (int argc, char *argv[])
case OPTION_KEEP_SYMBOLS:
  add_specific_symbols (optarg, _specific_list);
  break;
+
+   case OPTION_LOCALIZE_HIDDEN:
+ localize_hidden = TRUE;
+ break
 
case OPTION_LOCALIZE_SYMBOLS:
  add_specific_symbols (optarg, _specific_list);
Index: doc/binutils.texi

map stack problem with "homemade" stackguard

2018-04-16 Thread Sebastien Marie
Hi,

I recently tried to run qemu, and it failed with SIGABRT and dmesg
contains:

map stack 0x86277000-0x86378000 of map 0xd5343afc failed: bad protection
map stack for pid 43079 failed

I initially think about missing MAP_STACK. And effectively, the flag is
missing on the port (function qemu_alloc_stack() in util/oslib-posix.c).
But in fact, it seems it isn't underline problem of my failure: even
with MAP_STACK, the program is still aborted at the same place.

The real problem is qemu implements a stackguard protection

from qemu, util/oslib-posix.c (with manually added MAP_STACK):
   540  ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
   541 MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
   542  if (ptr == MAP_FAILED) {
   543  perror("failed to allocate memory for stack");
   544  abort();
   545  }
   546
   547  #if defined(HOST_IA64)
   548  /* separate register stack */
   549  guardpage = ptr + (((*sz - pagesz) / 2) & ~pagesz);
   550  #elif defined(HOST_HPPA)
   551  /* stack grows up */
   552  guardpage = ptr + *sz - pagesz;
   553  #else
   554  /* stack grows down */
   555  guardpage = ptr;
   556  #endif
   557
   558  if (mprotect(guardpage, pagesz, PROT_NONE) != 0) {
   559  perror("failed to set up stack guard page");
   560  abort();
   561  }
   562

and the kernel doesn't like it: in uvm_map_is_stack_remappable(), the
protection check is done by only allowing PROT_READ | PROT_WRITE:

sys/uvm/uvm_map.c
  1852  if (iter->protection != (PROT_READ | PROT_WRITE)) {
  1853  printf("map stack 0x%lx-0x%lx of map %p failed: 
"
  1854  "bad protection\n", addr, end, map);
  1855  return FALSE;
  1856  }

and the guardpage segment is PROT_NONE, resulting an error, and qemu
abort.

If I remove the mprotect(2) call from qemu, the program starts fine (I
didn't test it deeply).

Assuming the check on protection is a bit too restrictive, and it is
fine to have some chunks with PROT_NONE, I think we want something like:

Index: uvm/uvm_map.c
===
RCS file: /cvs/src/sys/uvm/uvm_map.c,v
retrieving revision 1.234
diff -u -p -r1.234 uvm_map.c
--- uvm/uvm_map.c   12 Apr 2018 17:13:44 -  1.234
+++ uvm/uvm_map.c   16 Apr 2018 15:20:30 -
@@ -1849,7 +1849,11 @@ uvm_map_is_stack_remappable(struct vm_ma
 #if 0
printf("iter prot: 0x%x\n", iter->protection);
 #endif
-   if (iter->protection != (PROT_READ | PROT_WRITE)) {
+   switch (iter->protection) {
+   case PROT_READ|PROT_WRITE:
+   case PROT_NONE:
+   break;
+   default:
printf("map stack 0x%lx-0x%lx of map %p failed: "
"bad protection\n", addr, end, map);
return FALSE;


I will send a diff for emulators/qemu to add MAP_STACK after this part
is resolved.

Thanks.
-- 
Sebastien Marie



  1   2   3   >