date:20181201

Re: uvm_fault: ip_ctloutput

2018-12-01 Thread Greg Steuck

This thwarts the reproducer. Again, I don't know if the invariants are
getting violated somewhere else and the patch below is simply papering over
the symptoms.

Please include with the fix:
Reported-by: syzbot+02168317bd0156c13...@syzkaller.appspotmail.com

diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index c963f7c5014..a430d2155cb 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -860,7 +860,7 @@ ip_ctloutput(int op, struct socket *so, int level, int
optname,
int error = 0;
u_int rtid = 0;

-   if (level != IPPROTO_IP) {
+   if (inp == NULL || level != IPPROTO_IP) {
error = EINVAL;
} else switch (op) {
case PRCO_SETOPT:

On Fri, Nov 30, 2018 at 8:08 PM Greg Steuck  wrote:

> The C reproducer panics the machine like a charm. Requires root.
> https://syzkaller.appspot.com/x/repro.c?x=117e573340
>
> -- Forwarded message -
> From: syzbot 
> Date: Fri, Nov 30, 2018 at 7:58 PM
> Subject: Re: uvm_fault: ip_ctloutput
> To: 
>
>
> syzbot has found a reproducer for the following crash on:
>
> HEAD commit:e9b93a3e5ebc Remove erroneous quote added in previous
> git tree:   https://github.com/openbsd/src.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=11a2362540
> dashboard link:
> https://syzkaller.appspot.com/bug?extid=02168317bd0156c13b69
> compiler:
> syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=111b11a340
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=117e573340
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+02168317bd0156c13...@syzkaller.appspotmail.com
>
> login: uvm_fault(0xff007f12bb58, 0xd0, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at  ip_ctloutput+0x784: movq0xd0(%r14),%rbx
> ddb>
> ddb> set $lines = 0
> ddb> show panic
> kernel page fault
> uvm_fault(0xff007f12bb58, 0xd0, 0, 1) -> e
> ip_ctloutput(ff006e48e170,8000210c2e20,ff006e706788,8000210fa988,ff007f146c00)
>
> at
> ip_ctloutput+0x784
> end trace frame: 0x8000210fa930, count: 0
> ddb> trace
> ip_ctloutput(ff006e48e170,8000210c2e20,ff006e706788,8000210fa988,ff007f146c00)
>
> at
> ip_ctloutput+0x784
> sys_getsockopt(8000210faa10,8000210c2e20,8000210a5338) at
> sys_getsockopt+0x13c
> syscall(0) at syscall+0x3e4
> Xsyscall(6,0,0,0,1,7f7f3a18) at Xsyscall+0x128
> end of kernel
> end trace frame: 0x7f7f39d0, count: -4
> ddb> show registers
> rdi0
> rsi   0xff006e706788
> rbp   0x8000210fa8d0
> rbx0
> rdx0
> rcx  0x1
> rax0
> r80xff007f146c00
> r9 0
> r10   0xa28679f43345c2df
> r11   0x8110e110rip_ctloutput
> r12  0x1
> r130
> r140
> r15   0xff007f146c00
> rip   0x81a13b44ip_ctloutput+0x784
> cs   0x8
> rflags   0x10246__ALIGN_SIZE+0xf246
> rsp   0x8000210fa8a0
> ss  0x10
> ip_ctloutput+0x784: movq0xd0(%r14),%rbx
> ddb> show proc
> PROC (syz-executor1283) pid=307178 stat=onproc
>  flags process=2 proc=0
>  pri=51, usrpri=51, nice=20
>  forw=0x, list=0x8000210c3078,0x81e98cf0
>  process=0x8000210a5338 user=0x8000210f5000,
> vmspace=0xff007f12bb58
>  estcpu=1, cpticks=1, pctcpu=0.0
>  user=0, sys=1, intr=0
> ddb> ps
> PID TID   PPIDUID  S   FLAGS  WAIT  COMMAND
> *22391  307178  19661  0  7 0x2syz-executor1283
>   19661  340086  17670  0  30x10008a  pause ksh
>   17670  326992  29604  0  30x92  selectsshd
>   41270   33654  1  0  30x100083  ttyin getty
>   29604  327245  1  0  30x80  selectsshd
>   79075   90932  56293 73  20x100090syslogd
>   56293  303628  1  0  30x100082  netio syslogd
>   68459  425749  1 77  30x100090  poll  dhclient
>   36911   58752  1  0  30x80  poll  dhclient
>   56206  238502  0  0  2 0x14200zerothread
>5835  239343  0  0  3 0x14200  aiodoned  aiodoned
>   38692  124704  0  0  3 0x14200  syncerupdate
>   30045  377418  0  0  3 0x14200  cleaner   cleaner
>8830  232312  0  0  3 0x14200  reaperreaper
>   36321  273872  0  0  3 0x14200  pgdaemon  pagedaemon
>   27140  184915  0  0  3 0x14200

Re: uvm_fault: sogetopt

2018-12-01 Thread Greg Steuck

Even though I have no idea what I'm doing, the patch below is enough to
thwart the reproducer. There are multiple places where the result of
sotounpcb is used without checking the result, but I don't know which
invariants are established non-locally.

Please do me a favor when committing this or a proper fix and heed
syzkaller's request:
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+2cd350dfe5c96f646...@syzkaller.appspotmail.com

--- a/sys/kern/uipc_socket.c
+++ b/sys/kern/uipc_socket.c
@@ -1905,6 +1905,8 @@ sogetopt(struct socket *so, int level, int optname,
struct mbuf *m)
case SO_PEERCRED:
if (so->so_proto->pr_protocol == AF_UNIX) {
struct unpcb *unp = sotounpcb(so);
+   if (unp == NULL)
+   return (EINVAL);

if (unp->unp_flags & UNP_FEIDS) {
m->m_len = sizeof(unp->unp_connid);


On Sat, Dec 1, 2018 at 3:13 PM Greg Steuck  wrote:

> This is the offending line:
>
> https://github.com/openbsd/src/blob/7c13478cbf7a624ad524dc377f8c2a7e497c0f3b/sys/kern/uipc_socket.c#L1909
> case SO_PEERCRED:
> if (so->so_proto->pr_protocol == AF_UNIX) {
> struct unpcb *unp = sotounpcb(so);
>
> * if (unp->unp_flags & UNP_FEIDS) {*
>
> I want to automate this whole objdump -dlr business, too much manual work.
>
>
>

-- 
nest.cx is Gmail hosted, use PGP for anything private. Key:
http://goo.gl/6dMsr
Fingerprint: 5E2B 2D0E 1E03 2046 BEC3  4D50 0B15 42BD 8DF5 A1B0

Re: apmd: -t: use strtonum()

2018-12-01 Thread Theo Buehler

On Sat, Dec 01, 2018 at 02:26:05PM +0100, Klemens Nanni wrote:
> On Sat, Dec 01, 2018 at 08:58:31AM +0100, Martijn van Duren wrote:
> > > I'm not sure the EINVAL error string adds valuable information.  I would
> > > prefer if all these used variants of the idiom suggested in the strtonum
> > > manual, something like:
> > > 
> > >   errx("number of seconds is %s: %s", errstr, optarg);
> > >   errx("battery percentage is %s: %s", errstr, optarg);
> > > 
> > That might be even better.
> I agree, thanks for the input.
> 
> OK?

ok

Re: option kcov + GENERIC.MP -> silent crash

2018-12-01 Thread Greg Steuck

Hi Anton,

Unfortunately it's still crashing. The log is below, but to make
sure I'm not deluding myself, the source tree is
https://github.com/blackgnezdo/src/tree/anton-kcov-dec1

This is the workdir where I'm building:

commit fea58d64a837907fd3b5c45eb2b77351ac105d5f (HEAD -> anton-kcov-dec1)

SYZKALLER.MP conf

commit 583b85f9857e44ee3339d9bb74e2780e435e3937 (origin/anton-kcov-dec1)

anton@: disable kcov in files incompatible with tracing

By performing a semi-automated bisect I was able to identify the source
files that are incompatible with tracing. Common for all source files
seems to be that they define routines called early on in the boot
process before curcpu() is usable.

commit 3f7c3e6a6fe644f1ab7c92ea63819fb056a99f6d

regen

Happy to test more patches. The easiest for me would be to merge
them as PRs into my github tree, but I'm happy to keep applying
them manually if it's more convenient for you.

Logs:

SeaBIOS (version 1.8.2-20181014_101610-google)
Total RAM Size = 0x0004 = 16384 MiB
CPUs found: 16 Max CPUs supported: 16
found virtio-scsi at 0:3
virtio-scsi vendor='Google' product='PersistentDisk' rev='1' type=0
removable=0
virtio-scsi blksize=512 sectors=20971520 = 10240 MiB
drive 0x000f2c00: PCHS=0/0/0 translation=lba LCHS=1024/255/63 s=20971520
Booting from Hard Disk 0...
>> OpenBSD/amd64 BOOT 3.41
boot> b bsd.anton

[ using 1964296 bytes of bsd ELF symbol table ]
Copyright (c) 1982, 1986, 1989, 1991, 1993
The Regents of the University of California.  All rights reserved.
Copyright (c) 1995-2018 OpenBSD. All rights reserved.
https://www.OpenBSD.org

OpenBSD 6.4-current (SYZKALLER.MP) #0: Sat Dec  1 10:27:33 PST 2018
syzkaller@ci-openbsd.syzkaller
:/home/syzkaller/s/src/sys/arch/amd64/compile/SYZKALLER.MP
real mem = 17163079680 (16367MB)
avail mem = 16632180736 (15861MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xbb80 (28 entries)
bios0: vendor Google version "Google" date 01/01/2011
bios0: Google Google Compute Engine
acpi0 at bios0: rev 0
acpi0: sleep states S3 S4 S5
acpi0: tables DSDT FACP SSDT APIC WAET SRAT
acpi0: wakeup devices
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Xeon(R) CPU @ 2.30GHz, 2300.63 MHz, 06-3f-00
cpu0:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SS,HTT,SSE3,PCLMUL,VMX,SSSE3,FMA3,CX16,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,ARAT,XSAVEOPT,MELTDOWN
cpu0: 256KB 64b/line 8-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 989MHz
cpu1 at mainbus0: apid 2 (application processor)
cpu1: Intel(R) Xeon(R) CPU @ 2.30GHz, 2276.74 MHz, 06-3f-00
cpu1:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SS,HTT,SSE3,PCLMUL,VMX,SSSE3,FMA3,CX16,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,ARAT,XSAVEOPT,MELTDOWN
cpu1: 256KB 64b/line 8-way L2 cache
cpu1: smt 0, core 1, package 0
cpu2 at mainbus0: apid 4 (application processor)
cpu2: Intel(R) Xeon(R) CPU @ 2.30GHz, 2276.75 MHz, 06-3f-00
cpu2:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SS,HTT,SSE3,PCLMUL,VMX,SSSE3,FMA3,CX16,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,ARAT,XSAVEOPT,MELTDOWN
cpu2: 256KB 64b/line 8-way L2 cache
cpu2: smt 0, core 2, package 0
cpu3 at mainbus0: apid 6 (application processor)
cpu3: Intel(R) Xeon(R) CPU @ 2.30GHz, 2276.74 MHz, 06-3f-00
cpu3:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SS,HTT,SSE3,PCLMUL,VMX,SSSE3,FMA3,CX16,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,ARAT,XSAVEOPT,MELTDOWN
cpu3: 256KB 64b/line 8-way L2 cache
cpu3: smt 0, core 3, package 0
cpu4 at mainbus0: apid 8 (application processor)
cpu4: Intel(R) Xeon(R) CPU @ 2.30GHz, 2276.74 MHz, 06-3f-00
cpu4:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SS,HTT,SSE3,PCLMUL,VMX,SSSE3,FMA3,CX16,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,ARAT,XSAVEOPT,MELTDOWN
cpu4: 256KB 64b/line 8-way L2 cache
cpu4: smt 0, core 4, package 0
cpu5 at mainbus0: apid 10 (application processor)
cpu5: Intel(R) Xeon(R) CPU @ 2.30GHz, 2276.76 MHz, 06-3f-00
cpu5:
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,C

Re: option kcov + GENERIC.MP -> silent crash

2018-12-01 Thread Martin Pieuchot

On 01/12/18(Sat) 16:34, Anton Lindqvist wrote:
> On Tue, Nov 27, 2018 at 05:52:15PM -0800, Greg Steuck wrote:
> > I booted the patched kernel and it seems to have gone farther and I believe
> > reached init before crashing.
> 
> By performing a semi-automated bisect I was able to identify the source
> files that are incompatible with tracing. Common for all source files
> seems to be that they define routines called early on in the boot
> process before curcpu() is usable.
> 
> I do not have any plans on committing the diff below but please give it
> a try. Instead, I'm working on extending the files.conf(5) grammar in
> order to infer a different make target for the files.

Is it possible to mark incompatible functions using __attribute__ and
the preprocessor?  For example offending code with GPROF is marked with:

#define __noprof __attribute__((__no_instrument_function__))

> Index: arch/amd64/conf/Makefile.amd64
> ===
> RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
> retrieving revision 1.106
> diff -u -p -r1.106 Makefile.amd64
> --- arch/amd64/conf/Makefile.amd6430 Oct 2018 11:08:30 -  1.106
> +++ arch/amd64/conf/Makefile.amd641 Dec 2018 15:32:58 -
> @@ -151,7 +151,31 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
>   ${CC} ${CFLAGS} ${CPPFLAGS} ${PROF} -c vers.c
>  
>  .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> +amd64_mem.o: $S/arch/amd64/amd64/amd64_mem.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +cpu.o: $S/arch/amd64/amd64/cpu.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +fpu.o: $S/arch/amd64/amd64/fpu.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +gdt.o: $S/arch/amd64/amd64/gdt.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +intr.o: $S/arch/amd64/amd64/intr.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +lapic.o: $S/arch/amd64/amd64/lapic.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +machdep.o: $S/arch/amd64/amd64/machdep.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +tsc.o: $S/arch/amd64/amd64/tsc.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
>  kcov.o: $S/dev/kcov.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +pvbus.o: $S/dev/pv/pvbus.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +kern_lock.o: $S/kern/kern_lock.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +kern_sched.o: $S/kern/kern_sched.c
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> +kern_tc.o: $S/kern/kern_tc.c
>   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
>  .endif
>  
>

Re: athn(4) hostap: mem leak

2018-12-01 Thread Martin Pieuchot

On 30/11/18(Fri) 13:49, Benjamin Baier wrote:
> Hi
> 
> There is a leak of *arg in 
> dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263
> since Rev. 1.49
> Because athn_usb_do_async() memcpy's the argument anyway.
> 
> Found with llvm/scan-build.
> 
> Instead of adding free(arg) I opted to make this function
> more like the other ones which call athn_usb_do_async.
> 
> Only compile tested... looking for tests.

You should also remove the free(arg...) in athn_usb_newauth_cb().

> Index: if_athn_usb.c
> ===
> RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 if_athn_usb.c
> --- if_athn_usb.c 6 Sep 2018 11:50:54 -   1.51
> +++ if_athn_usb.c 29 Nov 2018 18:33:40 -
> @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic
>   struct ifnet *ifp = &ic->ic_if;
>   struct athn_node *an = (struct athn_node *)ni;
>   int nsta;
> - struct athn_usb_newauth_cb_arg *arg;
> + struct athn_usb_newauth_cb_arg arg;
>  
>   if (ic->ic_opmode != IEEE80211_M_HOSTAP)
>   return 0;
> @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic
>* In a process context, try to add this node to the
>* firmware table and confirm the AUTH request.
>*/
> - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT);
> - if (arg == NULL)
> - return ENOMEM;
> - arg->ni = ieee80211_ref_node(ni);
> - arg->seq = seq;
> - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg));
> + arg.ni = ieee80211_ref_node(ni);
> + arg.seq = seq;
> + athn_usb_do_async(usc, athn_usb_newauth_cb, &arg, sizeof(arg));
>   return EBUSY;
>  #else
>   return 0;
>

Re: option kcov + GENERIC.MP -> silent crash

2018-12-01 Thread Anton Lindqvist

On Tue, Nov 27, 2018 at 05:52:15PM -0800, Greg Steuck wrote:
> I booted the patched kernel and it seems to have gone farther and I believe
> reached init before crashing.

By performing a semi-automated bisect I was able to identify the source
files that are incompatible with tracing. Common for all source files
seems to be that they define routines called early on in the boot
process before curcpu() is usable.

I do not have any plans on committing the diff below but please give it
a try. Instead, I'm working on extending the files.conf(5) grammar in
order to infer a different make target for the files.

Index: arch/amd64/conf/Makefile.amd64
===
RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
retrieving revision 1.106
diff -u -p -r1.106 Makefile.amd64
--- arch/amd64/conf/Makefile.amd64  30 Oct 2018 11:08:30 -  1.106
+++ arch/amd64/conf/Makefile.amd64  1 Dec 2018 15:32:58 -
@@ -151,7 +151,31 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
${CC} ${CFLAGS} ${CPPFLAGS} ${PROF} -c vers.c
 
 .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
+amd64_mem.o: $S/arch/amd64/amd64/amd64_mem.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+cpu.o: $S/arch/amd64/amd64/cpu.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+fpu.o: $S/arch/amd64/amd64/fpu.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+gdt.o: $S/arch/amd64/amd64/gdt.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+intr.o: $S/arch/amd64/amd64/intr.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+lapic.o: $S/arch/amd64/amd64/lapic.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+machdep.o: $S/arch/amd64/amd64/machdep.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+tsc.o: $S/arch/amd64/amd64/tsc.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
 kcov.o: $S/dev/kcov.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+pvbus.o: $S/dev/pv/pvbus.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+kern_lock.o: $S/kern/kern_lock.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+kern_sched.o: $S/kern/kern_sched.c
+   ${NORMAL_C} -fno-sanitize-coverage=trace-pc
+kern_tc.o: $S/kern/kern_tc.c
${NORMAL_C} -fno-sanitize-coverage=trace-pc
 .endif

Re: apmd: -t: use strtonum()

2018-12-01 Thread Klemens Nanni

On Sat, Dec 01, 2018 at 08:58:31AM +0100, Martijn van Duren wrote:
> > I'm not sure the EINVAL error string adds valuable information.  I would
> > prefer if all these used variants of the idiom suggested in the strtonum
> > manual, something like:
> > 
> > errx("number of seconds is %s: %s", errstr, optarg);
> > errx("battery percentage is %s: %s", errstr, optarg);
> > 
> That might be even better.
I agree, thanks for the input.

OK?

Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.82
diff -u -p -r1.82 apmd.c
--- apmd.c  30 Nov 2018 18:05:31 -  1.82
+++ apmd.c  1 Dec 2018 13:19:55 -
@@ -392,9 +392,10 @@ main(int argc, char *argv[])
sockname = optarg;
break;
case 't':
-   ts.tv_sec = strtoul(optarg, NULL, 0);
-   if (ts.tv_sec == 0)
-   usage();
+   ts.tv_sec = strtonum(optarg, 1, LLONG_MAX, &errstr);
+   if (errstr != NULL)
+   errx(1, "number of seconds is %s: %s", errstr,
+   optarg);
break;
case 's':   /* status only */
statonly = 1;
@@ -422,14 +423,14 @@ main(int argc, char *argv[])
autoaction = AUTO_HIBERNATE;
autolimit = strtonum(optarg, 1, 100, &errstr);
if (errstr != NULL)
-   errc(1, EINVAL, "%s percentage: %s", errstr,
+   errx(1, "battery percentage is %s: %s", errstr,
optarg);
break;
case 'z':
autoaction = AUTO_SUSPEND;
autolimit = strtonum(optarg, 1, 100, &errstr);
if (errstr != NULL)
-   errc(1, EINVAL, "%s percentage: %s", errstr,
+   errx(1, "battery percentage is %s: %s", errstr,
optarg);
break;
case '?':

Re: athn(4) hostap: mem leak

2018-12-01 Thread Alexandre Ratchov

On Sat, Dec 01, 2018 at 10:14:38AM +0100, Benjamin Baier wrote:
> On Fri, 30 Nov 2018 16:55:42 +0100
> Alexandre Ratchov  wrote:
> 
> > On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote:
> > > Hi
> > > 
> > > There is a leak of *arg in 
> > > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263
> > > since Rev. 1.49
> > > Because athn_usb_do_async() memcpy's the argument anyway.
> > > 
> > > Found with llvm/scan-build.
> > > 
> > > Instead of adding free(arg) I opted to make this function
> > > more like the other ones which call athn_usb_do_async.
> > >   
> > 
> > Hi,
> > 
> > AFAICS, athn_usb_do_async() will schedule a call to
> > athn_usb_newauth_cb(), which will use arg after the functin has
> > returned. The arg memory location must stay valid after return from
> > athn_usb_newauth(). So we can neither use free() nor a local variable.
> 
> athn_usb_do_async() takes care of that by memcpy-ing arg to cmd->data
> before calling usb_add_task().
> 
> other calls to athn_usb_do_async() do use local variables.
> if_athn_usb.c:1032:athn_usb_do_async(usc, athn_usb_newstate_cb, &cmd, 
> sizeof(cmd));
> if_athn_usb.c:1317:athn_usb_do_async(usc, athn_usb_ampdu_tx_start_cb, &cmd, 
> sizeof(cmd));
> if_athn_usb.c:1641:athn_usb_do_async(usc, athn_usb_set_key_cb, &cmd, 
> sizeof(cmd));
> if_athn_usb.c:1673:athn_usb_do_async(usc, athn_usb_delete_key_cb, &cmd, 
> sizeof(cmd));
> 

You're right, I missed the memcpy() call, sorry.

Your diff is correct.

Re: athn(4) hostap: mem leak

2018-12-01 Thread Benjamin Baier

On Fri, 30 Nov 2018 16:55:42 +0100
Alexandre Ratchov  wrote:

> On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote:
> > Hi
> > 
> > There is a leak of *arg in 
> > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263
> > since Rev. 1.49
> > Because athn_usb_do_async() memcpy's the argument anyway.
> > 
> > Found with llvm/scan-build.
> > 
> > Instead of adding free(arg) I opted to make this function
> > more like the other ones which call athn_usb_do_async.
> >   
> 
> Hi,
> 
> AFAICS, athn_usb_do_async() will schedule a call to
> athn_usb_newauth_cb(), which will use arg after the functin has
> returned. The arg memory location must stay valid after return from
> athn_usb_newauth(). So we can neither use free() nor a local variable.

athn_usb_do_async() takes care of that by memcpy-ing arg to cmd->data
before calling usb_add_task().

other calls to athn_usb_do_async() do use local variables.
if_athn_usb.c:1032:athn_usb_do_async(usc, athn_usb_newstate_cb, &cmd, 
sizeof(cmd));
if_athn_usb.c:1317:athn_usb_do_async(usc, athn_usb_ampdu_tx_start_cb, &cmd, 
sizeof(cmd));
if_athn_usb.c:1641:athn_usb_do_async(usc, athn_usb_set_key_cb, &cmd, 
sizeof(cmd));
if_athn_usb.c:1673:athn_usb_do_async(usc, athn_usb_delete_key_cb, &cmd, 
sizeof(cmd));

> The athn_usb_newauth_cb() callback calls free(), so there's no memory
> leak unless the callback is cancelled. I don't know it can be
> cancelled, I see no code doing so.
> 
> > Only compile tested... looking for tests.
> > 
> > Greetings Ben
> > 
> > Index: if_athn_usb.c
> > ===
> > RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v
> > retrieving revision 1.51
> > diff -u -p -r1.51 if_athn_usb.c
> > --- if_athn_usb.c   6 Sep 2018 11:50:54 -   1.51
> > +++ if_athn_usb.c   29 Nov 2018 18:33:40 -
> > @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic
> > struct ifnet *ifp = &ic->ic_if;
> > struct athn_node *an = (struct athn_node *)ni;
> > int nsta;
> > -   struct athn_usb_newauth_cb_arg *arg;
> > +   struct athn_usb_newauth_cb_arg arg;
> >  
> > if (ic->ic_opmode != IEEE80211_M_HOSTAP)
> > return 0;
> > @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic
> >  * In a process context, try to add this node to the
> >  * firmware table and confirm the AUTH request.
> >  */
> > -   arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT);
> > -   if (arg == NULL)
> > -   return ENOMEM;
> > -   arg->ni = ieee80211_ref_node(ni);
> > -   arg->seq = seq;
> > -   athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg));
> > +   arg.ni = ieee80211_ref_node(ni);
> > +   arg.seq = seq;
> > +   athn_usb_do_async(usc, athn_usb_newauth_cb, &arg, sizeof(arg));
> > return EBUSY;
> >  #else
> > return 0;
> >   
>

Re: uvm_fault: ip_ctloutput

Re: uvm_fault: sogetopt

Re: apmd: -t: use strtonum()

Re: option kcov + GENERIC.MP -> silent crash

Re: option kcov + GENERIC.MP -> silent crash

Re: athn(4) hostap: mem leak

Re: option kcov + GENERIC.MP -> silent crash

Re: apmd: -t: use strtonum()

Re: athn(4) hostap: mem leak

Re: athn(4) hostap: mem leak

10 matches

Site Navigation

Mail list logo

Footer information