Re: some vulns
CVSROOT:/cvs Module name:src Changes by: morti...@cvs.openbsd.org2020/02/15 15:59:55 Modified files: sys/arch/amd64/amd64: vmm.c Log message: Add bounds check on addresses passed from guests in pvclock. Fixes an issue where a guest can write to host memory by passing bogus addresses. I'm a bit confused here. It is not because the GPAs are contiguous that the HPAs are too. If the structure crosses a page, the guest still can write to host memory.
some vulns
In vmm_update_pvclock(): 6868pvclock_gpa = vcpu->vc_pvclock_system_gpa & 0xFFF0; <-- controlled by the guest 6869if (!pmap_extract(vm->vm_map->pmap, pvclock_gpa, _hpa)) 6870return (EINVAL); 6871pvclock_ti = (void*) PMAP_DIRECT_MAP(pvclock_hpa); 6872 6873/* START next cycle (must be odd) */ 6874pvclock_ti->ti_version = 6875(++vcpu->vc_pvclock_version << 1) | 0x1; Three things are wrong: 1) The RO protections are not enforced, so the guest could have data be written to a GPA it can only access as RO. 2) If 'pvclock_ti' crosses a page, its second half could point to an HPA that doesn't belong to the guest. The guest can therefore, to some limited extent, overwrite host kernel memory. 3) The pmap is not locked, so if the GPA gets unmapped and its corresponding HPA recycled, there is a small window where the (new) content of the HPA can get overwritten. There is, in fact, a fourth case. Watch closely. On AMD CPUs the NPTs are a regular pmap. The higher half of the GPA space is therefore mapped to host kernel memory as KVA. Given that there is no check on PG_u here, the guest can just put a host KVA in pvclock_gpa, and have its content be overwritten. This gives write-where ability for the guest. The OpenBSD kernel does not perform full ASLR, in that the PTE space and direct map are at static addresses (contrary to eg NetBSD where everything is randomized). These addresses are known. The guest can therefore use the static address of the direct map for example to write at whatever HPA by issuing the following instruction: wrmsr(KVM_MSR_SYSTEM_TIME, PMAP_DIRECT_MAP(hpa) | 1); This means the guest can overwrite whatever host kernel memory, and can control *where* to write. I have tested this, and it works. The guest can also choose *what* to write, because it just so happens that 'vc_pvclock_version' is the number of VMEXITs that occurred with pvclock enabled, and the guest can reliably craft this value. So this is not just a write-where, this is a full guest-to-host write-what-where. Had there been proper ASLR, it still could have been somewhat bypassable, because VMD does a pass-through of RDMSR on AMD CPUs (??), which can leak HPAs such as HSAVE_PA. (Speaking about direct map, notice how an alignment bug in locore0.S causes the first 2MB of .text to be writable on Intel CPUs. So there is a static address that maps the kernel .text as writable.) There are additional assorted bugs and vulns that could be used to some degree: - On AMD CPUs the CPL check on XSETBV VMEXITs must be performed by software. VMD forgot to do that, so from guest-userland, we can control the XCR0 that guest-kernel will use. - This XSETBV issue actually has an additional ramification. Right now OpenBSD doesn't check that the guest XCR0 is a subset of the host XCR0, which means that the guest can use more FPU states than the host allows. It looks like this check was lost when fixing another bug I reported one year ago which could cause guest-to-host DoS. - The TLB handling of guest pages is broken, in that the INVEPT instructions in the host could be issued on the wrong CPUs. This means that if UVM decides to swap out a guest page, the guest could still access it via stale TLB entries. On AMD CPUs, there is no TLB handling at all (??). - vmx_load_pdptes is broken. In order to make this whole thing less of a security joke, I would suggest the following: - Fix TLB handling, sanitize the GPAs, lock the pages correctly. - Don't pass-through RDMSR. - Fix the XSETBV issues. - Provide *real* ASLR: randomize the PTE space and the direct map. - Fix the alignment bug in the direct map to not map the text as writable. Maxime
Brainy: a few bugs
Got some time tonight; nothing new, just emptying my list: http://m00nbsd.net/e5ab5f6e59d6a0feb7d1a518acc8233d.html#Unsorted-2 Summary: _17/ UNINITIALIZED VARIABLE: sys/netinet/if_ether.c rev1.165 _18/ UNINITIALIZED VARIABLE: sys/net80211/ieee80211_pae_output.c rev1.20 _19/ UNINITIALIZED VARIABLE: sys/arch/i386/i386/bios.c rev1.112 _20/ UNINITIALIZED VARIABLE: sys/arch/sgi/dev/if_iec.c rev1.14 _21/ UNINITIALIZED VARIABLE: sys/arch/armv7/omap/ti_iic.c rev1.2 _22/ OVERLAP: sys/arch/sparc64/dev/vdsp.c rev1.2 _23/ USE-AFTER-FREE: sys/dev/sun/z8530ms.c rev1.2 _24/ MEMORY LEAK: sys/dev/ic/oosiop.c rev1.22 Found by Brainy. Maxime
Re: Brainy: User-Triggerable Kernel Memory Leak in execve()
Well, I guess I'll have to admit that I find your attitude extremely disrespectful. But I don't tend to feel particularly offended by this kind of things, so it probably does not matter. Le 21/07/2015 12:31, sam a écrit : On Tue, 21 Jul 2015 11:31:44 +0200 Maxime Villard m...@m00nbsd.net wrote: Found by The Brainy Code Scanner. It is not the last bug Brainy has found, but it is the last one I report. I don't have time for that. How about you release the Brainy Code Scanner then? I have so many bugs; in fact, there are so many, I don't even have the time to report them! My scanner is so good! Or perhaps you should report 'just' the relatively important ones? I think my work does (or used to) benefit to a lot of users, developers and vendors here; a lot of people, including you. Nobody supports my work, and I've never asked for anything here about that. Even though I almost never receive a simple thank you for all the bugs and vulnerabilities I've so far reported, I still expect a spiritual thank you for my work. But I certainly do not expect that kind of emails you just sent, somehow trying to either make fun of me or disregard what I'm willing to spend my spare time on for you. Developing, improving and maintaining Brainy takes time and energy, as well as investigating and packaging the bugs and vulnerabilities it finds. I've so far sent some reports here just because I'm friendly enough, and because modifying a few things for Brainy to properly understand the OpenBSD code does not require a Herculean work. Now, I believe that this effort is too much for my spare time. If you want to say thanks to me for reporting this vulnerability, dear Sam, it's never too late. Maxime
Brainy: Use-After-Free in if_bnx
Hi,
I put here a bug among others:
-- sys/dev/pci/if_bnx.c
if ((status L2_FHDR_STATUS_L2_VLAN_TAG)
!(sc-rx_mode BNX_EMAC_RX_MODE_KEEP_VLAN_TAG)) {
#if NVLAN 0
DBPRINT(sc, BNX_VERBOSE_SEND,
%s(): VLAN tag = 0x%04X\n,
__FUNCTION__,
l2fhdr-l2_fhdr_vlan_tag);
m-m_pkthdr.ether_vtag =
l2fhdr-l2_fhdr_vlan_tag;
m-m_flags |= M_VLANTAG;
#else
m_freem(m);
goto bnx_rx_int_next_rx;
#endif
}
bnx_rx_int_next_rx:
sw_prod = NEXT_RX_BD(sw_prod);
}
sw_cons = NEXT_RX_BD(sw_cons);
/* If we have a packet, pass it up the stack */
if (m) {
sc-rx_cons = sw_cons;
DBPRINT(sc, BNX_VERBOSE_RECV,
%s(): Passing received frame up.\n, __FUNCTION__);
ml_enqueue(ml, m);
DBRUNIF(1, sc-rx_mbuf_alloc--);
sw_cons = sc-rx_cons;
}
Use-after-free with 'm'.
Found by The Brainy Code Scanner.
Maxime
Brainy: User-Triggerable Kernel Memory Leak in execve()
Hi,
I put here a bug among others:
- sys/kern/kern_exec.c -
char *pathbuf = NULL;
[...]
pathbuf = pool_get(namei_pool, PR_WAITOK);
[...]
/* setup new registers and do misc. setup. */
if (pack.ep_emul-e_fixup != NULL) {
if ((*pack.ep_emul-e_fixup)(p, pack) != 0)
goto free_pack_abort;
}
[...]
free_pack_abort:
free(pack.ep_hdr, M_EXEC, 0);
exit1(p, W_EXITCODE(0, SIGABRT), EXIT_NORMAL);
/* NOTREACHED */
atomic_clearbits_int(pr-ps_flags, PS_INEXEC);
if (pathbuf != NULL)
pool_put(namei_pool, pathbuf);
return (0);
}
'pathbuf' is leaked.
This path being obviously reachable from userland, it is easy for a
local (un)privileged user to cause the kernel to run out of memory and
become unresponsive. OpenBSD 5.7 is affected, and quite certainly
previous releases.
Exploit here:
http://m00nbsd.net/garbage/OpenBSD_execve-DoS.txt
You can see with vmstat -m that the namei pool becomes enormous.
Found by The Brainy Code Scanner.
It is not the last bug Brainy has found, but it is the last one I
report. I don't have time for that.
Maxime
Brainy: Uninitialized Var in hppa64
Hi,
I put here a bug among others:
--- sys/arch/hppa64/dev/apic.c -
struct evcount *cnt;
struct apic_iv *aiv, *biv;
void *iv;
int irq = APIC_INT_IRQ(ih);
int line = APIC_INT_LINE(ih);
u_int32_t ent0;
/* no mapping or bogus */
if (irq = 0 || irq 63)
return (NULL);
aiv = malloc(sizeof(struct apic_iv), M_DEVBUF, M_NOWAIT);
if (aiv == NULL) {
free(cnt, M_DEVBUF, 0);
return NULL;
}
'cnt' is not initialized.
Found by The Brainy Code Scanner.
Maxime
Brainy: Use-After-Free in if_et
Hi,
I put here a bug among others:
-- sys/dev/pci/if_et.c -
1808if (m_defrag(m, M_DONTWAIT)) {
m_freem(m);
printf(%s: can't defrag TX mbuf\n,
sc-sc_dev.dv_xname);
error = ENOBUFS;
goto back;
}
[...]
back:
if (error) {
m_freem(m);
*m0 = NULL;
}
Use-after-free with 'm'.
Found by The Brainy Code Scanner.
Maxime
Brainy: Memory Leak in ti
Hi,
I put here a bug among others:
sys/dev/ic/ti.c ---
648 MGETHDR(m_new, M_DONTWAIT, MT_DATA);
if (m_new == NULL)
return (ENOBUFS);
m_new-m_len = m_new-m_pkthdr.len = MHLEN;
m_adj(m_new, ETHER_ALIGN);
if (bus_dmamap_load_mbuf(sc-sc_dmatag, dmamap, m_new,
BUS_DMA_NOWAIT)) {
m_freem(m);
return (ENOBUFS);
}
'm_new' is leaked.
Found by The Brainy Code Scanner.
Maxime
Brainy: Memory Leak in vax
Hi, I put here a bug among others: sys/arch/vax/if/if_qe.c --- 146 ring = malloc(PROBESIZE, M_TEMP, M_WAITOK | M_ZERO); bzero(sc, sizeof(struct qe_softc)); sc-sc_iot = ua-ua_iot; sc-sc_ioh = ua-ua_ioh; sc-sc_dmat = ua-ua_dmat; ubasc-uh_lastiv -= 4; QE_WCSR(QE_CSR_CSR, QE_RESET); QE_WCSR(QE_CSR_VECTOR, ubasc-uh_lastiv); /* * Map the ring area. Actually this is done only to be able to * send and receive a internal packet; some junk is loopbacked * so that the DEQNA has a reason to interrupt. */ ui.ui_size = PROBESIZE; ui.ui_vaddr = (caddr_t)ring[0]; if ((error = uballoc((void *)parent, ui, UBA_CANTWAIT))) return 0; 'ring' is leaked. Found by The Brainy Code Scanner. Maxime
Brainy: Memory Leak in ipmi
Hi,
I put here a bug among others:
sys/dev/ipmi.c
1046buf = malloc(maxlen + 3, M_DEVBUF, M_NOWAIT);
if (buf == NULL) {
printf(%s: ipmi_recvcmd: malloc fails\n, DEVNAME(sc));
return (-1);
}
/* Receive message from interface, copy out result data */
if (sc-sc_if-recvmsg(sc, maxlen + 3, rawlen, buf) ||
rawlen IPMI_MSG_DATARCV)
return (-1);
'buf' is leaked.
Found by The Brainy Code Scanner.
Maxime
Brainy: Memory Leak in rasops
Hi, I put here a bug among others: --- sys/dev/rasops/rasops.c 1167f = malloc(sizeof(struct rotatedfont), M_DEVBUF, M_WAITOK); if ((ncookie = wsfont_rotate(*cookie)) == -1) return; 'f' is leaked. Found by The Brainy Code Scanner. Maxime
Brainy: No Error Set in an
Hi,
I put here a bug among others:
--- sys/dev/ic/an.c
1030if (an_write_rid(sc, AN_RID_SSIDLIST, sc-sc_buf,
sizeof(sc-sc_buf.sc_ssidlist)) != 0) {
printf(%s: failed to write ssid list\n, ifp-if_xname);
an_stop(ifp, 1);
return error;
}
'error' does not hold an error code. Same problem l.1057.
Found by The Brainy Code Scanner.
Maxime
Brainy: Memory Leak in ICMP
Hi, I put here a bug among others: -- netinet/ip_icmp.c -- 925 rt = rtalloc(sintosa(sin), RT_REPORT|RT_RESOLVE, rtableid); if (rt == NULL) return (NULL); /* Check if the route is actually usable */ if (rt-rt_flags (RTF_REJECT | RTF_BLACKHOLE) || (rt-rt_flags RTF_UP) == 0) return (NULL); --- 'rt' is not released. Found by The Brainy Code Scanner. Maxime
Brainy: Kernel Use-after-free Memory Leak in hifn
Hi,
I put here two bugs among others:
sys/dev/pci/hifn7751.c
2757
if (!(m0-m_flags M_EXT))
m_freem(m0);
len = MCLBYTES;
totlen -= len;
m0-m_pkthdr.len = m0-m_len = len;
mlast = m0;
Use-after-free with 'm0'.
sys/dev/pci/hifn7751.c
2766
MGET(m, M_DONTWAIT, MT_DATA);
if (m == NULL) {
m_freem(m0);
return (NULL);
}
MCLGET(m, M_DONTWAIT);
if (!(m-m_flags M_EXT)) {
m_freem(m0);
return (NULL);
}
len = MCLBYTES;
'm' is leaked.
Found by The Brainy Code Scanner.
Maxime
Re: kernel patch available
Le 30/04/2015 22:42, Philip Guenther a écrit :
Patches are now available for 5.6 and 5.7 which fix local security
issues in the kernel's handling of malformed ELF executables, which
could be used to panic the kernel or view some kernel memory.
Our thanks to Alejandro Hernandez for test cases and Maxime Villard
for providing the basis for one of the changes.
Hum. It curiously looks like the patch I sent here about two years
ago that nobody gave a shit about:
http://marc.info/?l=openbsd-techm=138703150604223w=2
- - - -
And don't you feel like there's an obvious bug in these patches?
if (ph[i].p_filesz ph[i].p_memsz)
goto bad1;
[...]
bad1:
VOP_CLOSE(nd.ni_vp, FREAD, p-p_ucred, p);
bad:
free(ph, M_TEMP, 0);
*last = addr;
vput(nd.ni_vp);
return (error);
Where is 'error' initialized?
Le 30/04/2015 22:42, Philip Guenther a écrit :
Patches are now available for 5.6 and 5.7 which fix local security
issues in the kernel's handling of malformed ELF executables, which
could be used to panic the kernel or view some kernel memory.
Our thanks to Alejandro Hernandez for test cases and Maxime Villard
for providing the basis for one of the changes.
Links:
http://www.openbsd.org/errata56.html
http://ftp.openbsd.org/pub/OpenBSD/patches/5.6/common/023_elf.patch.sig
and
http://www.openbsd.org/errata57.html
http://ftp.openbsd.org/pub/OpenBSD/patches/5.7/common/006_elf.patch.sig
untrusted comment: signature from openbsd 5.7 base secret key
RWSvUZXnw9gUby4OBLM0n2MCFo9TM/FWZlryKfa4mLnPMEgi87dSLa8HTEXN15Z0YumeDyfsnFVHyQHjtL6106R1LxIOtJ/6pww=
OpenBSD 5.7 errata 6, Apr 30, 2015:
Missing validity checks in the kernel ELF loader meant malformed binaries
could trigger kernel panics or view kernel memory.
Apply by doing:
cd /usr/src
signify -Vep /etc/signify/openbsd-57-base.pub -x 006_elf.patch.sig -m - | \
patch -p0
Then build and install a new kernel:
cd /usr/src/sys/arch/`machine`/conf
KK=`sysctl -n kern.osversion | cut -d# -f1`
config $KK
cd ../compile/$KK
make
make install
Index: sys/kern/exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.112
diff -u -p -r1.112 exec_elf.c
--- sys/kern/exec_elf.c 10 Feb 2015 23:39:57 - 1.112
+++ sys/kern/exec_elf.c 30 Apr 2015 18:41:25 -
@@ -362,6 +362,8 @@ ELFNAME(load_file)(struct proc *p, char
for (i = 0; i eh.e_phnum; i++) {
if (ph[i].p_type == PT_LOAD) {
+ if (ph[i].p_filesz ph[i].p_memsz)
+ goto bad1;
loadmap[idx].vaddr = trunc_page(ph[i].p_vaddr);
loadmap[idx].memsz = round_page (ph[i].p_vaddr +
ph[i].p_memsz - loadmap[idx].vaddr);
@@ -549,14 +551,20 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz 2 || pp-p_filesz MAXPATHLEN)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
goto bad;
}
+ if (interp[pp-p_filesz - 1] != '\0')
+ goto bad;
} else if (pp-p_type == PT_LOAD) {
+ if (pp-p_filesz pp-p_memsz) {
+ error = EINVAL;
+ goto bad;
+ }
if (base_ph == NULL)
base_ph = pp;
} else if (pp-p_type == PT_PHDR) {
Brainy: Memory Leak in aic
Hi,
I put here a bug among others:
- sys/dev/ic/aic6915.c -
386 MGETHDR(m, M_DONTWAIT, MT_DATA);
if (m == NULL) {
printf(%s: unable to allocate Tx mbuf\n,
sc-sc_dev.dv_xname);
break;
}
if (m0-m_pkthdr.len MHLEN) {
MCLGET(m, M_DONTWAIT);
if ((m-m_flags M_EXT) == 0) {
printf(%s: unable to allocate Tx
cluster\n, sc-sc_dev.dv_xname);
m_freem(m);
break;
}
}
m_copydata(m0, 0, m0-m_pkthdr.len, mtod(m, caddr_t));
m-m_pkthdr.len = m-m_len = m0-m_pkthdr.len;
error = bus_dmamap_load_mbuf(sc-sc_dmat, dmamap,
m, BUS_DMA_WRITE|BUS_DMA_NOWAIT);
if (error) {
printf(%s: unable to load Tx buffer,
error = %d\n, sc-sc_dev.dv_xname, error);
break;
}
'm' is leaked.
Found by The Brainy Code Scanner.
Maxime
Brainy: Kernel Memory Leak in PF
Hi,
I put here a bug among others:
-- sys/net/pf_ioctl.c --
1027qs = pool_get(pf_queue_pl, PR_WAITOK|PR_LIMITFAIL|PR_ZERO);
if (qs == NULL) {
error = ENOMEM;
break;
}
bcopy(q-queue, qs, sizeof(*qs));
qs-qid = pf_qname2qid(qs-qname, 1);
if (qs-parent[0] (qs-parent_qid =
pf_qname2qid(qs-parent, 0)) == 0)
return (ESRCH);
'qs' is leaked.
Found by The Brainy Code Scanner.
Maxime
Brainy: Kernel Memory Leak in sdmmc
Hi, I put here a bug among others: -- dev/sdmmc/sdmmc.c -- 783 data = malloc(ucmd-c_datalen, M_TEMP, M_WAITOK | M_CANFAIL); if (data == NULL) return ENOMEM; if (copyin(ucmd-c_data, data, ucmd-c_datalen)) return EFAULT; --- 'data' is leaked. Found by The Brainy Code Scanner. Maxime
Re: Brainy: User-Triggerable Kernel Memory Leak
FWIW, it would be wise to propagate the fix to the stable branch(es).
I guess people use compat-linux.
Le 31/01/2015 00:33, Alexander Bluhm a écrit :
On Fri, Jan 30, 2015 at 03:57:12PM -0700, Todd C. Miller wrote:
On Fri, 30 Jan 2015 22:55:06 +0100, Alexander Bluhm wrote:
sosetopt() calls m_free() and then it is called again. So it is a
double free.
Whoops, I didn't notice that the non-error case also falls thought
to the bad label. We could just do what sys_setsockopt() does
and zero out m after calling sosetopt().
Let's take this fix. OK bluhm@
- todd
Index: sys/compat/linux/linux_socket.c
===
RCS file: /cvs/src/sys/compat/linux/linux_socket.c,v
retrieving revision 1.59
diff -u -r1.59 linux_socket.c
--- sys/compat/linux/linux_socket.c 21 Jan 2015 13:47:45 - 1.59
+++ sys/compat/linux/linux_socket.c 30 Jan 2015 22:56:40 -
@@ -969,10 +969,8 @@
if (lsa.optval != NULL) {
m = m_get(M_WAIT, MT_SOOPTS);
error = copyin(lsa.optval, mtod(m, caddr_t), lsa.optlen);
-if (error) {
-(void) m_free(m);
+if (error)
goto bad;
-}
m-m_len = lsa.optlen;
}
so = (struct socket *)fp-f_data;
@@ -984,7 +982,10 @@
goto bad;
}
error = sosetopt(so, level, name, m);
+m = NULL;
bad:
+if (m)
+m_free(m);
FRELE(fp, p);
return (error);
}
Brainy: User-Triggerable Kernel Memory Leak
Hi,
I put here a bug among others:
-- sys/compat/linux/linux_socket.c --
969 if (lsa.optval != NULL) {
m = m_get(M_WAIT, MT_SOOPTS);
error = copyin(lsa.optval, mtod(m, caddr_t), lsa.optlen);
if (error) {
(void) m_free(m);
goto bad;
}
m-m_len = lsa.optlen;
}
so = (struct socket *)fp-f_data;
if (so-so_proto level == IPPROTO_TCP name == TCP_NODELAY
so-so_proto-pr_domain-dom_family == AF_LOCAL
so-so_proto-pr_protocol == PF_LOCAL) {
/* ignore it */
error = 0;
goto bad;
}
error = sosetopt(so, level, name, m);
bad:
FRELE(fp, p);
return (error);
-
'm' is allocated and filled in, but later the function may jump to 'bad' and
return without freeing it.
'lsa' being user-controllable, it is easy for a local (un)privileged user to
cause the kernel to run out of memory and become unresponsive. OpenBSD 5.6/i386
is affected, and perhaps previous releases.
Exploit here:
http://m00nbsd.net/garbage/OpenBSD_Linux_DoS.c
Binary sample:
http://m00nbsd.net/garbage/OpenBSD_Linux_DoS.tar.gz
Found by my code scanner.
Maxime
Brainy: OpenSSH Memory Leak
Hi,
I put here a bug among others:
Index: ssh-ed25519.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh-ed25519.c,v
retrieving revision 1.4
diff -u -r1.4 ssh-ed25519.c
--- ssh-ed25519.c 24 Jun 2014 01:13:21 - 1.4
+++ ssh-ed25519.c 29 Aug 2014 10:28:35 -
@@ -125,8 +125,10 @@
r = SSH_ERR_INVALID_FORMAT;
goto out;
}
- if (datalen = SIZE_MAX - len)
- return SSH_ERR_INVALID_ARGUMENT;
+ if (datalen = SIZE_MAX - len) {
+ r = SSH_ERR_INVALID_ARGUMENT;
+ goto out;
+ }
smlen = len + datalen;
mlen = smlen;
if ((sm = malloc(smlen)) == NULL || (m = xmalloc(mlen)) == NULL) {
Found by my code scanner.
Maxime
Re: exec_elf.c: mistake ?
Well, at the time I didn't really know these stuff - this bug was found by my home-made code scanner, in an area I was not particularly familiar with. However, I did try to help as far as I could, but despite my pings, it has remained unfixed so far, and 5.5 was released with no care taken about that. I vaguely remember that I later ended up with a working patch, but apparently I've lost it since. This bug crashed binaries, and I had told myself it was annoying enough to be worth an Erratum. Anyway, it ain't that bad, and I'm glad to see people waking up one year later. (it won't be necessary to cc' me again for this issue) Regards, Maxime Le 17/06/2014 00:29, Matthew Dempsky a écrit : Reading through exec_elf.c, I just noticed the uninitialized bdiff variable myself, and remembered this thread. Tangentially, the code for worrying about zero-filling the last page is overzealous. We only need to zero-fill the page if memsz filesz (accounting for alignment and page boundaries). In the common case (at least on amd64), we always have either memsz==filesz (most segments) or filesz==0 (segment for .bss), so we shouldn't need zero-filling. And actually, I think the logic for only doing zero-fill for writable segments is wrong. It looks to me like the ELF gABI specifies zero-fill semantics for memsz filesz even for read-only segments. On Sat, Oct 5, 2013 at 4:27 PM, Philip Guenther guent...@gmail.com wrote: On Thu, 15 Aug 2013, Maxime Villard wrote: Hum hum, after investigating a bit, and from what I understood - if I'm not mistaken, I think it would make sense if bdiff was set to bdiff = ph-p_vaddr - trunc_page(ph-p_vaddr); Yep. Since we are supposed to align only on 2^n boundaries, if we get 0 or 1 we do not align on p_align. But p_vaddr still has to be aligned on PAGE_MASK (with trunc_page()); I read somewhere that ELF loaders are smart enough to adjust the address when it does not exactly fit a page boundary. So bdiff should be the difference with the original p_vaddr. Actually, bdiff is already set to this value in the other conditions. There's another problem, 'base' should also be initialized here. I would say that is should be set to the truncated p_vaddr plus the address at which we want to load: base = *addr + trunc_page(ph-p_vaddr); but I'm not sure at all. By the logic of the ph-p_align 1 case, it should be base = *addr + trunc_page(ph-p_vaddr) - ph-p_vaddr; (I think the only reason the 'if' is necessary is that p_align==0 must be treated the same as p_align==1. The ELF_TRUNC() macro handles an alignment of 1 correctly, but it'll barf on 0.) Philip Guenther
Re: [patch] courier-imap-4.13 imapd patch replacing malloc, strcat and strcpy with asprintf
Le 20/04/2014 02:38, Peter Malone a écrit :
Hi,
I'm using OpenBSD 5.5. courier-imap-4.13 is in the ports tree and it's
quite a mess. I started looking at it today with the hope of just
replacing some of the malloc,strcat strcpy calls with asprintf, but it
became clear before long that there's lots more issues with this code.
Regardless, here's a patch which fixes some of the malloc,strcat
strcpy spaghetti in imapd.c
I figure you guys are fairly busy with OpenSSL right now so if it's OK
with you I'll get working on the rest of this code.
I spoke with the courier-imap team and their response was less than
satisfactory - claiming asprintf code won't compile on non-linux
platforms.
courier-imap is *totally buggy*. Not only because there are big
programming mistakes - at a time I started to fix many issues like
double-free, use-after-free, leaks, etc., before letting down -,
but also because this team doesn't give a fuck at all.
Give a quick glance at the code: it is just a great mess, with bugs
everywhere, no indentation, and probably no review. Clearly, there
are poor - not to say *no* - considerations for security. That's the
idea I made myself of this project, and you seem to have come to the
same conclusion.
--- imapd_orig.c Sat Apr 19 19:38:18 2014
+++ imapd.c Sat Apr 19 20:09:48 2014
@@ -343,9 +343,8 @@
if (q)
{
- r=malloc(strlen(q)+sizeof(/.));
- if (!r) write_error_exit(0);
- strcat(strcpy(r, q), /.);
+ if(asprintf(r, %s%s, q, /.) == -1)
+ write_error_exit(0);
if (access(r, 0) == 0)
{
free(r);
@@ -1373,11 +1372,9 @@
static char *alloc_filename(const char *mbox, const char *name)
{
-char *p=malloc(strlen(mbox)+strlen(name)+sizeof(/cur/));
-
- if (!p) write_error_exit(0);
-
- strcat(strcat(strcpy(p, mbox), /cur/), name);
+ char *p;
+ if(asprintf(p, %s%s%s, mbox, /cur/, name) == -1)
+ write_error_exit(0);
return (p);
}
@@ -1812,14 +1809,12 @@
{
#if EXPLICITDIRSYNC
- char *p=malloc(strlen(folder)+sizeof(/new));
+ char *p;
int fd;
-
- if (!p)
+
+ if(asprintf(p, %s%s, folder, /new) == -1)
write_error_exit(0);
- p=strcat(strcpy(p, folder), /new);
-
fd=open(p, O_RDONLY);
if (fd = 0)
@@ -1828,7 +1823,8 @@
close(fd);
}
- p=strcat(strcpy(p, folder), /cur);
+ if(asprintf(p, %s%s, folder, /cur) == -1)
+ write_error_exit(0);
fd=open(p, O_RDONLY);
@@ -2212,12 +2208,10 @@
{
struct imapscaninfo minfo;
- char *p=malloc(strlen(new_path)+sizeof(/ IMAPDB));
+ char *p;
+ if(asprintf(p, %s%s, new_path, / IMAPDB) == -1)
+write_error_exit(0);
- if (!p)
- write_error_exit(0);
-
- strcat(strcpy(p, new_path), / IMAPDB);
unlink(p);
free(p);
imapscan_init(minfo);
@@ -2448,14 +2442,12 @@
}
return 0;
}
- q=malloc(strlen(p)+sizeof(/new));
- if (!q)
+ if(asprintf(q, %s%s, p, /new) == -1)
{
free(p);
maildir_aclt_list_destroy(aclt_list);
return -1;
}
- strcat(strcpy(q, p), /new);
free(p);
if (maildir_aclt_list_add(aclt_list,
@@ -4216,11 +4208,10 @@
if (p (p=maildir_shareddir(., p+1)) != NULL)
{
int rc;
- char *q=malloc(strlen(p)+sizeof(/shared));
-
- if (!q) write_error_exit(0);
-
- strcat(strcpy(q, p), /shared);
+ char *q;
+
+ if(asprintf(q, %s%s, p, /shared) == -1)
+ write_error_exit(0);
free(p);
rc=append(tag, tok-tokenbuf, q);
free(q);
@@ -6041,10 +6032,9 @@
isshared=0;
if (is_sharedsubdir(cqinfo.destmailbox))
{
- char *p=malloc(strlen(cqinfo.destmailbox)+sizeof(/shared));
-
- if (!p) write_error_exit(0);
- strcat(strcpy(p, cqinfo.destmailbox), /shared);
+ char *p;
+ if(asprintf(p, %s%s, cqinfo.destmailbox, /shared) == -1)
+ write_error_exit(0);
free(mailbox);
mailbox=cqinfo.destmailbox=p;
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
I would like to end this thread. Here's a final patch:
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 26 Nov 2013 18:36:35 -
@@ -552,11 +552,16 @@
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz 2 || pp-p_filesz MAXPATHLEN)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is NUL-terminated and of the expected
length */
+ if (strnlen(interp, pp-p_filesz) != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
To sum up what it does, and why it does what it does:
PT_INTERP segments are parts of PIE binaries, and specify a path to
an interpreter. This path's len is registered in the p_filesz field
of this segment, which includes the last NUL byte. The goal of this patch
is to check the length of this path to ensure it is consistent with
what is specified in p_filesz.
We do this, because otherwise the kernel may use a non-NUL-terminated
interp. It currently doesn't harm, since the copystr()'s that are later
called will stop the execution as well. But it is error-prone.
Ok/Opinions?
Re: Weird loop in ftp client
Is there something wrong with this patch?
Le 29/11/2013 20:44, Maxime Villard a écrit :
What about that?
Index: ftp.c
===
RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.83
diff -u -r1.83 ftp.c
--- ftp.c 13 Nov 2013 20:41:14 - 1.83
+++ ftp.c 29 Nov 2013 21:17:49 -
@@ -1089,9 +1089,10 @@
may_send_noop_char();
d = 0;
do {
- wr = write(fileno(fout), buf + d, rd);
- if (wr == -1 errno == EPIPE)
+ if ((wr = write(fileno(fout), buf + d, rd)) ==
-1) {
+ d = -1;
break;
+ }
d += wr;
rd -= wr;
} while (d c);
p is set to -1 to display the proper error message
of course here I meant 'd'
linux_socket: uninitialized variable ~ NULL pointer dereference
Hi, I was reading linux_socket.c when I came across a bug in linux_sendmsg(). Index: linux_socket.c === RCS file: /cvs/src/sys/compat/linux/linux_socket.c,v retrieving revision 1.46 diff -u -r1.46 linux_socket.c --- linux_socket.c 26 Jun 2012 10:18:08 - 1.46 +++ linux_socket.c 14 Dec 2013 17:45:37 - @@ -1214,7 +1214,7 @@ struct msghdr msg, *nmsg = NULL; int error; caddr_t control; - int level; + int level = -1; if ((error = copyin((caddr_t) uap, (caddr_t) lla, sizeof lla))) return error; At l.1252, if control == NULL, the function jumps to 'done' and 'level' is checked while it hasn't been initialized. As 'control' is NULL, copyout() tries to write to NULL-cmsg_level. What the kernel wants to do is to translate the level from 1 to SOL_SOCKET, call OpenBSD's sendmsg(), and then reset it to 1. By setting level to -1, we prevent it from entering the if in 'done'. Ok/Comments?
Re: linux_socket: uninitialized variable ~ NULL pointer dereference
Le 14/12/2013 19:18, Ted Unangst a écrit :
On Sat, Dec 14, 2013 at 19:06, Maxime Villard wrote:
Hi,
I was reading linux_socket.c when I came across a bug in
linux_sendmsg().
At l.1252, if control == NULL, the function jumps to 'done' and 'level'
is checked while it hasn't been initialized. As 'control' is NULL,
copyout() tries to write to NULL-cmsg_level.
Agree with the patch. Can you do one other change? Change
control-level to control + offsetof(level). (Maybe with a second
variable.) I think that would make it more clear that we're not
dereferencing the pointer in the kernel and would reduce the number of
casts.
ah, something like that? (not tested)
Index: linux_socket.c
===
RCS file: /cvs/src/sys/compat/linux/linux_socket.c,v
retrieving revision 1.46
diff -u -r1.46 linux_socket.c
--- linux_socket.c 26 Jun 2012 10:18:08 - 1.46
+++ linux_socket.c 14 Dec 2013 19:30:03 -
@@ -1214,7 +1214,8 @@
struct msghdr msg, *nmsg = NULL;
int error;
caddr_t control;
- int level;
+ int level = -1;
+ void *ulevel;
if ((error = copyin((caddr_t) uap, (caddr_t) lla, sizeof lla)))
return error;
@@ -1251,8 +1252,8 @@
return error;
if (control == NULL)
goto done;
- error = copyin(((struct cmsghdr *)control)-cmsg_level,
- level, sizeof(int));
+ ulevel = control + offsetof(struct cmsghdr, cmsg_level);
+ error = copyin(ulevel, level, sizeof(int));
if (error)
return error;
if (level == 1) {
@@ -1266,8 +1267,7 @@
* We don't because the control header is variable length
* up to 2048 bytes, and there's only 512 bytes of gap.
*/
- error = copyout(level, ((struct cmsghdr *)control)-
- cmsg_level, sizeof(int));
+ error = copyout(level, ulevel, sizeof(int));
if (error)
return error;
}
@@ -1277,8 +1277,7 @@
if (level == SOL_SOCKET) {
level = 1;
/* don't worry about the error */
- copyout(level, ((struct cmsghdr *)control)-cmsg_level,
- sizeof(int));
+ copyout(level, ulevel, sizeof(int));
}
return (error);
}
Kernel leak with compat-linux
Hi,
when loading a linux binary, the kernel could leak MAXPATHLEN bytes.
Index: linux_exec.c
===
RCS file: /cvs/src/sys/compat/linux/linux_exec.c,v
retrieving revision 1.38
diff -u -r1.38 linux_exec.c
--- linux_exec.c3 Nov 2013 13:52:44 - 1.38
+++ linux_exec.c1 Dec 2013 17:33:55 -
@@ -227,9 +227,10 @@
if (itp) {
if ((error = emul_find(p, NULL, linux_emul_path, itp, bp, 0)))
return (error);
- if ((error = copystr(bp, itp, MAXPATHLEN, len)))
- return (error);
+ error = copystr(bp, itp, MAXPATHLEN, len);
free(bp, M_TEMP);
+ if (error)
+ return (error);
}
epp-ep_emul = emul_linux_elf;
*pos = ELF32_NO_ADDR;
emul_find() allocates bp, and if the copystr() fails, bp is lost.
However, there's apparently no reason to fail here, since bp isn't
larger than MAXPATHLEN and is NUL-terminated.
While here, also fix a typo in a comment:
Index: linux_exec.c
===
RCS file: /cvs/src/sys/compat/linux/linux_exec.c,v
retrieving revision 1.38
diff -u -r1.38 linux_exec.c
--- linux_exec.c3 Nov 2013 13:52:44 - 1.38
+++ linux_exec.c2 Dec 2013 17:40:11 -
@@ -218,7 +218,7 @@
* If this is a static binary, do not allow it to run, as it
* has not been identified. We'll give non-static binaries a
* chance to run, as the Linux ld.so name is usually unique
-* enough to clear any amibiguity.
+* enough to clear any ambiguity.
*/
if (itp == NULL)
return (EINVAL);
Ok/Comments?
Re: Weird loop in ftp client
What about that?
Index: ftp.c
===
RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.83
diff -u -r1.83 ftp.c
--- ftp.c 13 Nov 2013 20:41:14 - 1.83
+++ ftp.c 29 Nov 2013 21:17:49 -
@@ -1089,9 +1089,10 @@
may_send_noop_char();
d = 0;
do {
- wr = write(fileno(fout), buf + d, rd);
- if (wr == -1 errno == EPIPE)
+ if ((wr = write(fileno(fout), buf + d, rd)) ==
-1) {
+ d = -1;
break;
+ }
d += wr;
rd -= wr;
} while (d c);
p is set to -1 to display the proper error message
Weird loop in ftp client
Hi,
a thing I spotted some weeks ago
- - - usr.bin/ftp/ftp.c l.1090 - - -
d = 0;
do {
wr = write(fileno(fout), buf + d, rd);
if (wr == -1 errno == EPIPE)
break;
d += wr;
rd -= wr;
} while (d c);
If write() fails without EPIPE, d is decremented, and the function
keeps looping. If write() succeeds after several loops, d will be
negative, and the function will write from buf-XX.
Since d is later used to display a proper error message, I'd suggest
this patch:
Index: ftp.c
===
RCS file: /cvs/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.83
diff -u -r1.83 ftp.c
--- ftp.c 13 Nov 2013 20:41:14 - 1.83
+++ ftp.c 22 Nov 2013 06:23:32 -
@@ -1094,7 +1094,7 @@
break;
d += wr;
rd -= wr;
- } while (d c);
+ } while (d c d 0);
if (rd != 0)
break;
bytes += c;
Any opinion?
Re: Weird loop in ftp client
Le 22/11/2013 17:48, Ted Unangst a écrit : On Fri, Nov 22, 2013 at 10:09, Stuart Henderson wrote: On 2013/11/22 07:25, Maxime Villard wrote: If write() fails without EPIPE, d is decremented, and the function keeps looping. If write() succeeds after several loops, d will be negative, and the function will write from buf-XX. When does write() fail and do we want to keep writing? (If I fill up the filesystem, do I really want to spin here?) I must say that I actually fail to see why EPIPE is the only case handled; it should stop looping regardless of the errno code, shouldn't it? Shouldn't it be something more like this? Otherwise if the write() fails, we attempt writing one byte fewer for every retry. That looks better to me.
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
Le 06/10/2013 01:09, Kenneth R Westerback a écrit :
On Sat, Oct 05, 2013 at 03:22:36PM -0600, Todd C. Miller wrote:
On Wed, 28 Aug 2013 22:34:26 -0400, Kenneth R Westerback wrote:
@@ -552,11 +552,16 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz 2 || pp-p_filesz = MAXPATHLEN)
Still think you're depriving yourself of one character out by using
= instead of .
I'm not sure about this. We want to limit the path length to
MAXPATHLEN-1 since we include the NUL terminator in MAXPATHLEN.
The buffer we get from namei_pool is MAXPATHLEN long and the
read_from() function just calls vn_rdwr(). If we allow interp to
be MAXPATHLEN, is there any guarantee that it will end in a NUL
byte?
- todd
since get_pool() is not given PR_ZERO, interp won't be zeroed. So even
with the '=', there's no guarantee that it will end in a NUL byte.
p_filesz includes the last '\0'; there's no problem with changing '='
to ''.
My reading at the time convinced me that p_filesz also includes the NUL. So
using = left room for two NULs.
But I am not trying to hold up either version, since I don't really
understand the relevant code. :-)
Ken
Re: Fix error code for ELF
Le 05/10/2013 23:10, Todd C. Miller a écrit : That looks reasonable to me. I can't think of a reason to not return the proper errno values, especially as things like ENOMEM are already documented for execve(2). - todd up...
Re: Wrong rights for ELF interpreters
On 10/20/13 21:54, Theo de Raadt wrote: Indeed, the interpreter is not passed to execve. That's why I used 'get executed' instead of 'are executed' though the difference might not be clear. The kernel loads the interpreter, and the code of that interpreter gets executed. So, actually, it plays as an executable. And as long as code gets executed from it, it should have +x rights. Shouldn't it? Absolutely not, because then someone can try to run execve on it. Maybe I'm missing something. I don't get what's wrong with running execve on it. In all cases, someone can load it through another executable. If I have an interpreter that I chmod as exec-only, I want this interpreter to be world-loadable without thereby letting other users copy it. The same for a library. You are not thinking clearly. I've just given a glance to FreeBSD and NetBSD. They both check exec rights, not read rights. So it looks like I'm not the only one who does not think clearly...
Re: Wrong rights for ELF interpreters
Le 21/10/2013 09:38, Theo de Raadt a écrit : I don't get what's wrong with running execve on it. In all cases, someone can load it through another executable. Using ld.so does not imply execve'ing it. If I have an interpreter that I chmod as exec-only, I want this interpreter to be world-loadable without thereby letting other users copy it. The same for a library. It is loadable. Because it is readable. I said exec-only, so no it is not readable and not loadable. My point was that, actually, it should be loadable. But after sleeping a bit, I figured out that there may indeed be some issues with some libraries. To me it looks weird. I'm gonna investigate a bit, and if I have an objection I'll send it here. Until then, sorry for the noise.
Wrong rights for ELF interpreters
Hi, when the kernel loads an ELF binary, it will also load its interpreter. The kernel checks the rights of the interpreter, that way: if ((error = VOP_ACCESS(vp, VREAD, p-p_ucred, p)) != 0) goto bad1; It should check with VEXEC instead of VREAD. Interpreters get executed, so they have to be executable; a read-only interpreter shouldn't be loaded by the kernel. Index: exec_elf.c === RCS file: /cvs/src/sys/kern/exec_elf.c,v retrieving revision 1.93 diff -u -r1.93 exec_elf.c --- exec_elf.c 4 Jul 2013 17:37:05 - 1.93 +++ exec_elf.c 7 Oct 2013 19:03:33 - @@ -345,7 +345,7 @@ error = EACCES; goto bad; } - if ((error = VOP_ACCESS(vp, VREAD, p-p_ucred, p)) != 0) + if ((error = VOP_ACCESS(vp, VEXEC, p-p_ucred, p)) != 0) goto bad1; if ((error = ELFNAME(read_from)(p, nd.ni_vp, 0, (caddr_t)eh, sizeof(eh))) != 0) Ok/Comments?
Re: Wrong rights for ELF interpreters
Le 20/10/2013 16:53, Theo de Raadt a écrit : when the kernel loads an ELF binary, it will also load its interpreter. The kernel checks the rights of the interpreter, that way: if ((error = VOP_ACCESS(vp, VREAD, p-p_ucred, p)) != 0) goto bad1; It should check with VEXEC instead of VREAD. Interpreters get executed, so they have to be executable; a read-only interpreter shouldn't be loaded by the kernel. I am not sure I agree on this. Why?
Re: Wrong rights for ELF interpreters
Le 20/10/2013 18:05, Theo de Raadt a écrit : Le 20/10/2013 16:53, Theo de Raadt a écrit : when the kernel loads an ELF binary, it will also load its interpreter. The kernel checks the rights of the interpreter, that way: if ((error = VOP_ACCESS(vp, VREAD, p-p_ucred, p)) != 0) goto bad1; It should check with VEXEC instead of VREAD. Interpreters get executed, so they have to be executable; a read-only interpreter shouldn't be loaded by the kernel. I am not sure I agree on this. Why? VEXEC is used in other cases to insist on a filesystem permission, for instance, when supplying a path for execve(). The interpreter is not a path supplied to execve. Indeed, the interpreter is not passed to execve. That's why I used 'get executed' instead of 'are executed' though the difference might not be clear. The kernel loads the interpreter, and the code of that interpreter gets executed. So, actually, it plays as an executable. And as long as code gets executed from it, it should have +x rights. Shouldn't it?
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
up, please
Le 28/08/2013 21:43, Maxime Villard a écrit :
On 08/28/13 20:57, Matthew Dempsky wrote:
On Wed, Aug 28, 2013 at 5:54 AM, Maxime Villard m...@m00nbsd.net wrote:
+ /* Ensure interp is a valid, NUL-terminated string
*/
+ for (n = 0; n pp-p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
A more concise test would be:
if (strnlen(interp, pp-p_filesz) != pp-p_filesz - 1) {
error = ENOEXEC;
goto bad;
}
Hum you're right, it's better that way
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c28 Aug 2013 21:14:22 -
@@ -552,11 +552,16 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz 2 || pp-p_filesz = MAXPATHLEN)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is NUL-terminated and of the expected
length */
+ if (strnlen(interp, pp-p_filesz) != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
It no longer looks like my patch...
Re: Set of 14 potential bugs
Le 12/07/2013 20:24, Maxime Villard a écrit : Hi, as I did for NetBSD, here is a list of 14 potential bugs/errors found by my code scanner in OpenBSD: http://M00nBSD.net/e5ab5f6e59d6a0feb7d1a518acc8233d.html I do not provide patches. May I ask what's the status of the remaining bugs?
[PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
Hi,
in the ELF format, the PT_INTERP segment contains the path of the
interpreter which must be loaded. For this segment, the kernel looks
at these values in the program header:
p_offset: offset of the path string
p_filesz: size of the path string, including the \0
The path string must be a valid string, or the binary won't be loaded.
The kernel actually doesn't check the string, it just reads p_filesz
bytes from p_offset, and later it checks if it can be loaded. Malformed
binaries which have paths like:
a
or
aaa\0aaa\0aaa\0aaa\0
are parsed, loaded, and then the kernel figures out that the path is
not valid, and aborts.
When the kernel reads the path from the file, it should check directly
the string to ensure it is at least a valid string.
Here is a patch for this. For pp-p_filesz, I could have put
if (pp-p_filesz == 0)
but values of 1 also don't make sense; \0 can't be a valid path.
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 27 Aug 2013 22:59:21 -
@@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
Elf_Ehdr *eh = epp-ep_hdr;
Elf_Phdr *ph, *pp, *base_ph = NULL;
Elf_Addr phdr = 0, exe_base = 0;
- int error, i;
+ int error, i, n;
char *interp = NULL;
u_long pos = 0, phsize;
size_t randomizequota = ELF_RANDOMIZE_LIMIT;
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz = MAXPATHLEN ||
+ pp-p_filesz 2)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is a valid, NULL-terminated string */
+ for (n = 0; n pp-p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
If you want to test
* Before patching
take whatever binary you have, open it with a text/hex editor, at the
beginning of the file there should be a string like /usr/libexec/ld.so,
just add a character to it, then run the binary with execv(): you will
get abort traps.
* After patching
execv() now returns -1, and errno is set to ENOEXEC.
There should not be any regression; if there is, it means that the binary
is not spec compliant.
Ok/Comments?
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
Updated diff, with small tweaks from Andres Perera,
* int - size_t, signedness issue, even if it can't be INT_MAX
* NULL - NUL
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 28 Aug 2013 14:36:58 -
@@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
int error, i;
char *interp = NULL;
u_long pos = 0, phsize;
- size_t randomizequota = ELF_RANDOMIZE_LIMIT;
+ size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
if (epp-ep_hdrvalid sizeof(Elf_Ehdr))
return (ENOEXEC);
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz = MAXPATHLEN ||
+ pp-p_filesz 2)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is a valid, NUL-terminated string */
+ for (n = 0; n pp-p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
On 08/28/13 08:44, Maxime Villard wrote:
Hi,
in the ELF format, the PT_INTERP segment contains the path of the
interpreter which must be loaded. For this segment, the kernel looks
at these values in the program header:
p_offset: offset of the path string
p_filesz: size of the path string, including the \0
The path string must be a valid string, or the binary won't be loaded.
The kernel actually doesn't check the string, it just reads p_filesz
bytes from p_offset, and later it checks if it can be loaded. Malformed
binaries which have paths like:
a
or
aaa\0aaa\0aaa\0aaa\0
are parsed, loaded, and then the kernel figures out that the path is
not valid, and aborts.
When the kernel reads the path from the file, it should check directly
the string to ensure it is at least a valid string.
Here is a patch for this. For pp-p_filesz, I could have put
if (pp-p_filesz == 0)
but values of 1 also don't make sense; \0 can't be a valid path.
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c27 Aug 2013 22:59:21 -
@@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
Elf_Ehdr *eh = epp-ep_hdr;
Elf_Phdr *ph, *pp, *base_ph = NULL;
Elf_Addr phdr = 0, exe_base = 0;
- int error, i;
+ int error, i, n;
char *interp = NULL;
u_long pos = 0, phsize;
size_t randomizequota = ELF_RANDOMIZE_LIMIT;
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz = MAXPATHLEN ||
+ pp-p_filesz 2)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is a valid, NULL-terminated string */
+ for (n = 0; n pp-p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
If you want to test
* Before patching
take whatever binary you have, open it with a text/hex editor, at the
beginning of the file there should be a string like /usr/libexec/ld.so,
just add a character to it, then run the binary with execv(): you will
get abort traps.
* After patching
execv() now returns -1
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On 08/28/13 16:30, Kenneth R Westerback wrote:
On Wed, Aug 28, 2013 at 02:54:11PM +0200, Maxime Villard wrote:
Updated diff, with small tweaks from Andres Perera,
* int - size_t, signedness issue, even if it can't be INT_MAX
* NULL - NUL
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 28 Aug 2013 14:36:58 -
@@ -516,7 +516,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
int error, i;
char *interp = NULL;
u_long pos = 0, phsize;
-size_t randomizequota = ELF_RANDOMIZE_LIMIT;
+size_t n, randomizequota = ELF_RANDOMIZE_LIMIT;
if (epp-ep_hdrvalid sizeof(Elf_Ehdr))
return (ENOEXEC);
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
-if (pp-p_filesz = MAXPATHLEN)
+if (pp-p_filesz = MAXPATHLEN ||
+pp-p_filesz 2)
Since p_filesz includes the NUL and MAXPATHLEN includes the NUL as
far as I know, could the comparison not be simply ' MAXPATHLEN'?
In passing I note that 37 out of 749 declarations using MAXPATHLEN in the
tree use 'char blah[MAXPATHLEN + 1]'. No idea if it is worth looking at
making them do without the '+ 1'. :-)
I also note in passing several #define PATH_MAX (MAXPATHLEN + 1)'
declarations, which strike me as odd since MAXPATHLEN is defined
in sys/param.h by '#define MAXPATHEN PATH_MAX'.
Let the POSIX lawyers apply any necessary cluebats please.
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+goto bad;
+}
+/* Ensure interp is a valid, NUL-terminated string */
The condition is not that the string is NUL terminated (aaa\0aaa\0 is after
all NUL terminated and valid as far as any function reading strings would be
concerned. It just has trailing garbage.) Perhaps the comment should refer
to making sure the string is spec compliant by being of the expected length.
By 'valid string' I actually meant 'without trailing garbage'... but ok
if it's not clear
/* Ensure interp is NUL-terminated and of the expected length */
Ken
+for (n = 0; n pp-p_filesz; n++) {
+if (interp[n] == '\0')
+break;
+}
+if (n != pp-p_filesz - 1) {
+error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
On 08/28/13 08:44, Maxime Villard wrote:
Hi,
in the ELF format, the PT_INTERP segment contains the path of the
interpreter which must be loaded. For this segment, the kernel looks
at these values in the program header:
p_offset: offset of the path string
p_filesz: size of the path string, including the \0
The path string must be a valid string, or the binary won't be loaded.
The kernel actually doesn't check the string, it just reads p_filesz
bytes from p_offset, and later it checks if it can be loaded. Malformed
binaries which have paths like:
a
or
aaa\0aaa\0aaa\0aaa\0
are parsed, loaded, and then the kernel figures out that the path is
not valid, and aborts.
When the kernel reads the path from the file, it should check directly
the string to ensure it is at least a valid string.
Here is a patch for this. For pp-p_filesz, I could have put
if (pp-p_filesz == 0)
but values of 1 also don't make sense; \0 can't be a valid path.
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 27 Aug 2013 22:59:21 -
@@ -513,7 +513,7 @@ ELFNAME2(exec,makecmds)(struct proc *p,
Elf_Ehdr *eh = epp-ep_hdr;
Elf_Phdr *ph, *pp, *base_ph = NULL;
Elf_Addr phdr = 0, exe_base = 0;
- int error, i;
+ int error, i, n;
char *interp = NULL;
u_long pos = 0, phsize;
size_t randomizequota = ELF_RANDOMIZE_LIMIT;
@@ -552,11 +552,21 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz = MAXPATHLEN ||
+ pp-p_filesz 2)
goto
Re: [PATCH] ELF: ensure PT_INTERP strings are NULL-terminated
On 08/28/13 20:57, Matthew Dempsky wrote:
On Wed, Aug 28, 2013 at 5:54 AM, Maxime Villard m...@m00nbsd.net wrote:
+ /* Ensure interp is a valid, NUL-terminated string */
+ for (n = 0; n pp-p_filesz; n++) {
+ if (interp[n] == '\0')
+ break;
+ }
+ if (n != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
A more concise test would be:
if (strnlen(interp, pp-p_filesz) != pp-p_filesz - 1) {
error = ENOEXEC;
goto bad;
}
Hum you're right, it's better that way
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 28 Aug 2013 21:14:22 -
@@ -552,11 +552,16 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz 2 || pp-p_filesz = MAXPATHLEN)
goto bad;
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
+ goto bad;
+ }
+ /* Ensure interp is NUL-terminated and of the expected
length */
+ if (strnlen(interp, pp-p_filesz) != pp-p_filesz - 1) {
+ error = ENOEXEC;
goto bad;
}
} else if (pp-p_type == PT_LOAD) {
It no longer looks like my patch...
Fix error code for ELF
Hi,
after playing with some ELF binaries, I figured out that error
codes received in errno do not always match with what is expected
in the kernel.
Actually, this function should return (error) instead of (ENOEXEC):
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -p -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 26 Aug 2013 15:26:06 -
@@ -552,8 +552,10 @@ ELFNAME2(exec,makecmds)(struct proc *p,
for (i = 0, pp = ph; i eh-e_phnum; i++, pp++) {
if (pp-p_type == PT_INTERP !interp) {
- if (pp-p_filesz = MAXPATHLEN)
+ if (pp-p_filesz = MAXPATHLEN) {
+ error = ENOEXEC;
goto bad;
+ }
interp = pool_get(namei_pool, PR_WAITOK);
if ((error = ELFNAME(read_from)(p, epp-ep_vp,
pp-p_offset, interp, pp-p_filesz)) != 0) {
@@ -567,8 +569,10 @@ ELFNAME2(exec,makecmds)(struct proc *p,
if (eh-e_type == ET_DYN) {
/* need an interpreter and load sections for PIE */
- if (interp == NULL || base_ph == NULL)
+ if (interp == NULL || base_ph == NULL) {
+ error = ENOEXEC;
goto bad;
+ }
/* randomize exe_base for PIE */
exe_base = uvm_map_pie(base_ph-p_align);
}
@@ -589,9 +593,9 @@ ELFNAME2(exec,makecmds)(struct proc *p,
* *interp with a changed path (/emul/xxx/path), and also
* set the ep_emul field in the exec package structure.
*/
- error = ENOEXEC;
if (eh-e_ident[EI_OSABI] != ELFOSABI_OPENBSD
ELFNAME(os_pt_note)(p, epp, epp-ep_hdr, OpenBSD, 8, 4) != 0) {
+ error = ENOEXEC;
for (i = 0; ELFNAME(probes)[i].func != NULL error; i++)
error = (*ELFNAME(probes)[i].func)(p, epp, interp,
pos);
if (error)
@@ -760,7 +764,7 @@ bad:
pool_put(namei_pool, interp);
free(ph, M_TEMP);
kill_vmcmds(epp-ep_vmcmds);
- return (ENOEXEC);
+ return (error);
}
/*
Ok/Comments?
Re: exec_elf.c: mistake ?
Le 06/07/2013 17:42, Kenneth R Westerback a écrit :
On Sat, Jul 06, 2013 at 05:21:31PM +0200, Maxime Villard wrote:
Hi,
- - - - sys/kern/exec_elf.c l.236 ~ 251252
Are my code scanner and me wrong, or 'bdiff' may not be
initialized ?
Codewise it does look possible that bdiff will be used uninitialized.
Whether it can happen in reality depends on whether ph-p_align can ever
be 1. Next question -- what would the correct value for bdiff be in that
= 1
case? 0? i.e. should the line be 'diff = bdiff = 0;'.
Ken
Hum hum, after investigating a bit, and from what I understood - if I'm
not mistaken, I think it would make sense if bdiff was set to
bdiff = ph-p_vaddr - trunc_page(ph-p_vaddr);
Since we are supposed to align only on 2^n boundaries, if we get 0 or 1
we do not align on p_align. But p_vaddr still has to be aligned on
PAGE_MASK (with trunc_page()); I read somewhere that ELF loaders are
smart enough to adjust the address when it does not exactly fit a page
boundary. So bdiff should be the difference with the original p_vaddr.
Actually, bdiff is already set to this value in the other conditions.
There's another problem, 'base' should also be initialized here. I would
say that is should be set to the truncated p_vaddr plus the address at
which we want to load:
base = *addr + trunc_page(ph-p_vaddr);
but I'm not sure at all.
Anyway, here is a patch. With these changes, 'uaddr' becomes useless,
so I removed it.
...Ok?
Index: exec_elf.c
===
RCS file: /cvs/src/sys/kern/exec_elf.c,v
retrieving revision 1.93
diff -u -r1.93 exec_elf.c
--- exec_elf.c 4 Jul 2013 17:37:05 - 1.93
+++ exec_elf.c 14 Aug 2013 18:31:37 -
@@ -215,7 +215,7 @@
ELFNAME(load_psection)(struct exec_vmcmd_set *vcset, struct vnode *vp,
Elf_Phdr *ph, Elf_Addr *addr, Elf_Addr *size, int *prot, int flags)
{
- u_long uaddr, msize, lsize, psize, rm, rf;
+ u_long msize, lsize, psize, rm, rf;
long diff, offset, bdiff;
Elf_Addr base;
@@ -229,19 +229,16 @@
/* page align vaddr */
base = *addr + trunc_page(ph-p_vaddr)
- ELF_TRUNC(ph-p_vaddr, ph-p_align);
-
- bdiff = ph-p_vaddr - trunc_page(ph-p_vaddr);
-
} else
diff = 0;
} else {
- *addr = uaddr = ph-p_vaddr;
+ *addr = ph-p_vaddr;
if (ph-p_align 1)
- *addr = ELF_TRUNC(uaddr, ph-p_align);
- base = trunc_page(uaddr);
- bdiff = uaddr - base;
- diff = uaddr - *addr;
+ *addr = ELF_TRUNC(*addr, ph-p_align);
+ base = trunc_page(ph-p_vaddr);
+ diff = ph-p_vaddr - *addr;
}
+ bdiff = ph-p_vaddr - trunc_page(ph-p_vaddr);
*prot |= (ph-p_flags PF_R) ? VM_PROT_READ : 0;
*prot |= (ph-p_flags PF_W) ? VM_PROT_WRITE : 0;
pcmcia/if_ray.c: dead code
Hi, in src/sys/dev/pcmcia/if_ray.c, at l.1018: - - - - case SIOCG80211NWID: RAY_DPRINTF((%s: ioctl: cmd SIOCG80211NWID\n, ifp-if_xname)); error = copyout(sc-sc_cnwid, ifr-ifr_data, sizeof(sc-sc_cnwid)); break; -- #ifdef RAY_DO_SIGLEV error = copyout(sc-sc_siglevs, ifr-ifr_data, sizeof sc-sc_siglevs); break; #endif default: [...] - - - - The code in the 'ifdef' is never reached, because of the 'break'. Found by my scanner.
softraid_aoe.c
Hi, - - - dev/softraid_aoe.c - - - I don't know if it's a mistake, but at l.393 the function always returns 1. If you put that just to temporarily shut down the function, then you should add a comment to explicitly tell to the stupid guys who might think it's a mistake - like me - something like /* Ok here. */
Set of 14 potential bugs
Hi, as I did for NetBSD, here is a list of 14 potential bugs/errors found by my code scanner in OpenBSD: http://M00nBSD.net/e5ab5f6e59d6a0feb7d1a518acc8233d.html I do not provide patches.
Static variables
Hi,
is it normal that in some functions like
tc_ticktock(void)
{
static int count;
if (++count tc_tick)
return;
count = 0;
tc_windup();
}
the static variables are not initialized?
- kern_tc.c l.547
- kern_resource.c l.499
- kern_sched.c l.106
- subr_extent.c l.130
- ...
Re: Static variables
Le 08/07/2013 11:00, Franco Fichtner a écrit : Hi Maxime, On Jul 8, 2013, at 10:40 AM, Maxime Villard rusty...@gmx.fr wrote: the static variables are not initialized? Static variables are always zeroed when not specified otherwise. Regards, Franco Ah, yes. I didn't know.
exec_elf.c: mistake ?
Hi, - - - - sys/kern/exec_elf.c l.236 ~ 251252 Are my code scanner and me wrong, or 'bdiff' may not be initialized ?
smtpd: if?
Hi, - - - src/usr.sbin/smtpd/smtpd.c l.1326 if (! bsnprintf(key, sizeof key, profiling.imsg.%s.%s.%s, proc_name(smtpd_process), proc_name(p-proc), imsg_to_str(imsg-hdr.type)));-- ';' stat_set(key, stat_timespec(dt)); What's the goal of the 'if' right there?
[PATCH?] Variable assignments...
Hi,
there are lots of useless assignment of variables in the code. I know this
kind of things does not really matter, but when I run my code scanner on
some parts of the source tree it gives me lots of them.
For example, for the net* directories:
== src/sys/net/if_bridge.c - l2017
u_int32_t cnt = 0;--- Here, we don't need to set cnt to 0
struct bridge_rtnode *n;
struct ifbareq bareq;
if (baconf-ifbac_len == 0)
onlycnt = 1;
for (i = 0, cnt = 0; i BRIDGE_RTABLE_SIZE; i++) --- set here
== src/sys/net/if_sppprubr.c - l403
int i = 0, x;
i = 0; Hum, hum, hum
== src/sys/net/if_pppx.c - l238
int rv = 0; --- ?
rv = rw_enter(pppx_devs_lk, RW_WRITE | RW_INTR);
== src/sys/netinet/if_output.c - l623
int transportmode = 0; - ?
transportmode = (tdb-tdb_dst.sa.sa_family == AF_INET)
(tdb-tdb_dst.sin.sin_addr.s_addr ==
ip-ip_dst.s_addr);
== src/sys/netinet6/raw_ip6.c - l380
int priv = 0; --
va_list ap;
int flags;
va_start(ap, m);
so = va_arg(ap, struct socket *);
dstsock = va_arg(ap, struct sockaddr_in6 *);
control = va_arg(ap, struct mbuf *);
va_end(ap);
in6p = sotoin6pcb(so);
priv = 0; --- ?
Same thing in several other places... Here is a patch for these dirs.
Ok/Comments?
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.210
diff -u -r1.210 if_bridge.c
--- net/if_bridge.c 28 Mar 2013 23:10:05 - 1.210
+++ net/if_bridge.c 24 Jun 2013 15:55:08 -
@@ -2014,7 +2014,7 @@
bridge_rtfind(struct bridge_softc *sc, struct ifbaconf *baconf)
{
int i, error = 0, onlycnt = 0;
- u_int32_t cnt = 0;
+ u_int32_t cnt;
struct bridge_rtnode *n;
struct ifbareq bareq;
Index: net/if_pppx.c
===
RCS file: /cvs/src/sys/net/if_pppx.c,v
retrieving revision 1.23
diff -u -r1.23 if_pppx.c
--- net/if_pppx.c 24 Jun 2013 09:34:59 - 1.23
+++ net/if_pppx.c 24 Jun 2013 15:55:08 -
@@ -235,7 +235,7 @@
pppxopen(dev_t dev, int flags, int mode, struct proc *p)
{
struct pppx_dev *pxd;
- int rv = 0;
+ int rv;
rv = rw_enter(pppx_devs_lk, RW_WRITE | RW_INTR);
if (rv != 0)
Index: net/if_spppsubr.c
===
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.104
diff -u -r1.104 if_spppsubr.c
--- net/if_spppsubr.c 20 Jun 2013 12:03:40 - 1.104
+++ net/if_spppsubr.c 24 Jun 2013 15:55:09 -
@@ -4028,7 +4028,6 @@
STDDCL;
int i = 0, x;
- i = 0;
sp-rst_counter[IDX_CHAP] = sp-lcp.max_configure;
/*
Index: netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.241
diff -u -r1.241 ip_output.c
--- netinet/ip_output.c 11 Jun 2013 18:15:53 - 1.241
+++ netinet/ip_output.c 24 Jun 2013 15:55:10 -
@@ -620,7 +620,7 @@
tdb-tdb_mtutimeout time_second) {
struct rtentry *rt = NULL;
int rt_mtucloned = 0;
- int transportmode = 0;
+ int transportmode;
transportmode = (tdb-tdb_dst.sa.sa_family == AF_INET)
(tdb-tdb_dst.sin.sin_addr.s_addr ==
Index: netinet6/raw_ip6.c
===
RCS file: /cvs/src/sys/netinet6/raw_ip6.c,v
retrieving revision 1.58
diff -u -r1.58 raw_ip6.c
--- netinet6/raw_ip6.c 4 Jun 2013 19:11:52 - 1.58
+++ netinet6/raw_ip6.c 24 Jun 2013 15:55:10 -
@@ -377,7 +377,6 @@
in6p = sotoin6pcb(so);
- priv = 0;
if ((so-so_state SS_PRIV) != 0)
priv = 1;
dst = dstsock-sin6_addr;
[PATCH] Mistake in Intel/DRM...
I have been making a code scanner for a while, and I wanted to test
a new rule, so I scanned sys/pci/drm. It found an uninitialized
variable in intel_ddi.c.
Quite simple, if !(val DDI_BUF_CTL_ENABLE) at l.1420, the variable
'wait' is not initialized at l.1432.
I guess I should send it upstream, but, you know, I don't want to
subscribe to the ML only to send one small mail. Is there someone here
who's upstream or who's willing to send that patch?
Thanks
Index: intel_ddi.c
===
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_ddi.c,v
retrieving revision 1.1
diff -u -r1.1 intel_ddi.c
--- intel_ddi.c 18 Mar 2013 12:36:52 - 1.1
+++ intel_ddi.c 5 May 2013 10:56:40 -
@@ -1412,7 +1412,7 @@
struct intel_dp *intel_dp = intel_dig_port-dp;
struct inteldrm_softc *dev_priv = encoder-dev-dev_private;
enum port port = intel_dig_port-port;
- bool wait;
+ bool wait = false;
uint32_t val;
if (I915_READ(DP_TP_CTL(port)) DP_TP_CTL_ENABLE) {
[ntpd] Useless variables...
Hi,
here is a patch to remove two useless variables in ntpd. Spotted
when recompiling with -Wextra.
Ok/Comments?
Index: client.c
===
RCS file: /cvs/src/usr.sbin/ntpd/client.c,v
retrieving revision 1.89
diff -u -p -r1.89 client.c
--- client.c21 Sep 2011 15:41:30 - 1.89
+++ client.c27 Apr 2013 21:59:52 -
@@ -186,8 +186,7 @@ client_query(struct ntp_peer *p)
p-query-msg.xmttime.fractionl = arc4random();
p-query-xmttime = gettime_corrected();
- if (ntp_sendmsg(p-query-fd, NULL, p-query-msg,
- NTP_MSGSIZE_NOAUTH, 0) == -1) {
+ if (ntp_sendmsg(p-query-fd, NULL, p-query-msg) == -1) {
p-senderrors++;
set_next(p, INTERVAL_QUERY_PATHETIC);
p-trustlevel = TRUSTLEVEL_PATHETIC;
Index: ntp_msg.c
===
RCS file: /cvs/src/usr.sbin/ntpd/ntp_msg.c,v
retrieving revision 1.18
diff -u -p -r1.18 ntp_msg.c
--- ntp_msg.c 19 Oct 2007 15:53:57 - 1.18
+++ ntp_msg.c 27 Apr 2013 21:59:52 -
@@ -40,8 +40,7 @@ ntp_getmsg(struct sockaddr *sa, char *p,
}
int
-ntp_sendmsg(int fd, struct sockaddr *sa, struct ntp_msg *msg, ssize_t len,
-int auth)
+ntp_sendmsg(int fd, struct sockaddr *sa, struct ntp_msg *msg)
{
socklen_t sa_len;
ssize_t n;
Index: ntpd.h
===
RCS file: /cvs/src/usr.sbin/ntpd/ntpd.h,v
retrieving revision 1.106
diff -u -p -r1.106 ntpd.h
--- ntpd.h 20 Sep 2012 12:43:16 - 1.106
+++ ntpd.h 27 Apr 2013 21:59:52 -
@@ -226,7 +226,7 @@ struct ntp_conf_sensor *new_sensor(char
/* ntp_msg.c */
intntp_getmsg(struct sockaddr *, char *, ssize_t, struct ntp_msg *);
-intntp_sendmsg(int, struct sockaddr *, struct ntp_msg *, ssize_t, int);
+intntp_sendmsg(int, struct sockaddr *, struct ntp_msg *);
/* server.c */
intsetup_listeners(struct servent *, struct ntpd_conf *, u_int *);
Index: server.c
===
RCS file: /cvs/src/usr.sbin/ntpd/server.c,v
retrieving revision 1.36
diff -u -p -r1.36 server.c
--- server.c21 Sep 2011 15:41:30 - 1.36
+++ server.c27 Apr 2013 21:59:52 -
@@ -210,6 +210,6 @@ server_dispatch(int fd, struct ntpd_conf
reply.rootdelay = d_to_sfp(lconf-status.rootdelay);
reply.refid = lconf-status.refid;
- ntp_sendmsg(fd, (struct sockaddr *)fsa, reply, size, 0);
+ ntp_sendmsg(fd, (struct sockaddr *)fsa, reply);
return (0);
}
Re: [PATCH] tftpd: DoS vuln
Le 15/03/2013 12:17, David Gwynne a écrit :
On 15/03/2013, at 9:02 AM, Maxime Villard rusty...@gmx.fr wrote:
Hi,
there is a huge bug in the tftp daemon.
sure, but then there's also a huge bug in your fix by your own definition of
huge.
when oack fails it frees the client struct and everything hanging off it. now
you avoid the unconditional free of of the client options straight after oack
is called by going to the error label if oack fails, which calls nak, which
then uses client and tries to free it again.
No. I removed the call to client_free() in oack():
- client_free(client);
+ return -1;
There is no double-free in my patch. But I saw you committed
fixes, so ok now.
nice find though. i'll put a fix in shortly which will look very much like
yours.
dlg
-- /usr/src/usr.sbin/tftpd/tftpd.c --
In tftp_open() on line 870, the daemon checks for options
(OACK) and handle them through the oack() function. Then,
it frees and NULLs the variable client-options.
oack() - l.1390 - does some stuff, and if an error happens,
client_free() is called with the client structure, which
frees this structure, including client-options, but does
not null it.
So, when returning after an error, client-options is freed
twice, causing a double-free and a crash.
I succeeded in making the server crash, by changing the
tsize and blksize options before transferring a small file
in localhost. In fact, if you look on line 1432, you'll see
that you just have to close the socket to make the server
crash, just after sending the OACK packet.
Typically, I just have to do:
$ tftp localhost
tftp tsize 2
Tsize option on.
tftp blksize 10
tftp get test
[...wait ~ 10 sec...]
tftpd in free(): error: chunk is already free 0x1a658ca65f40
Here is a patch. I switched oack() to int, so that we can
handle errors and call nak() to alert the client before
closing the connection. nak() frees the client structure,
so all goes well.
Ok/Comments?
Index: tftpd.c
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.8
diff -u -r1.8 tftpd.c
--- tftpd.c 13 Jul 2012 02:31:46 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -168,7 +168,7 @@
void tftp(struct tftp_client *, struct tftphdr *, size_t);
void tftp_open(struct tftp_client *, const char *);
void nak(struct tftp_client *, int);
-voidoack(struct tftp_client *);
+int oack(struct tftp_client *);
void oack_done(int, short, void *);
void sendfile(struct tftp_client *);
@@ -876,8 +876,8 @@
goto error;
if (client-options) {
-oack(client);
-
+if (oack(client) == -1)
+goto error;
free(client-options);
client-options = NULL;
} else if (client-opcode == WRQ) {
@@ -1386,7 +1386,7 @@
/*
* Send an oack packet (option acknowledgement).
*/
-void
+int
oack(struct tftp_client *client)
{
struct opt_client *options = client-options;
@@ -1436,10 +1436,10 @@
oack_done, client);
event_add(client-sev, client-tv);
-return;
+return 0;
error:
-client_free(client);
+return -1;
}
int
Re: [PATCH] tftpd: DoS vuln
What would that be used for? Look: client_free() frees
client-options, client-file AND client. So even if
you null client-options, client does no longer exist
as it has been freed. We should never use client after
calling client_free().
On 03/15/13 01:09, Ted Unangst wrote:
After reading your description, I was expecting the patch to include a line
setting options to null after freeing it. Whatever else we do, shouldn't we
do that too?
On Fri, Mar 15, 2013 at 00:02, Maxime Villard wrote:
Hi,
there is a huge bug in the tftp daemon.
-- /usr/src/usr.sbin/tftpd/tftpd.c --
In tftp_open() on line 870, the daemon checks for options
(OACK) and handle them through the oack() function. Then,
it frees and NULLs the variable client-options.
oack() - l.1390 - does some stuff, and if an error happens,
client_free() is called with the client structure, which
frees this structure, including client-options, but does
not null it.
So, when returning after an error, client-options is freed
twice, causing a double-free and a crash.
I succeeded in making the server crash, by changing the
tsize and blksize options before transferring a small file
in localhost. In fact, if you look on line 1432, you'll see
that you just have to close the socket to make the server
crash, just after sending the OACK packet.
Typically, I just have to do:
$ tftp localhost
tftp tsize 2
Tsize option on.
tftp blksize 10
tftp get test
[...wait ~ 10 sec...]
tftpd in free(): error: chunk is already free 0x1a658ca65f40
Here is a patch. I switched oack() to int, so that we can
handle errors and call nak() to alert the client before
closing the connection. nak() frees the client structure,
so all goes well.
Ok/Comments?
Index: tftpd.c
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.8
diff -u -r1.8 tftpd.c
--- tftpd.c 13 Jul 2012 02:31:46 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -168,7 +168,7 @@
void tftp(struct tftp_client *, struct tftphdr *, size_t);
void tftp_open(struct tftp_client *, const char *);
void nak(struct tftp_client *, int);
-voidoack(struct tftp_client *);
+int oack(struct tftp_client *);
void oack_done(int, short, void *);
void sendfile(struct tftp_client *);
@@ -876,8 +876,8 @@
goto error;
if (client-options) {
-oack(client);
-
+if (oack(client) == -1)
+goto error;
free(client-options);
client-options = NULL;
} else if (client-opcode == WRQ) {
@@ -1386,7 +1386,7 @@
/*
* Send an oack packet (option acknowledgement).
*/
-void
+int
oack(struct tftp_client *client)
{
struct opt_client *options = client-options;
@@ -1436,10 +1436,10 @@
oack_done, client);
event_add(client-sev, client-tv);
-return;
+return 0;
error:
-client_free(client);
+return -1;
}
int
[PATCH] tftpd: DoS vuln
Hi,
there is a huge bug in the tftp daemon.
-- /usr/src/usr.sbin/tftpd/tftpd.c --
In tftp_open() on line 870, the daemon checks for options
(OACK) and handle them through the oack() function. Then,
it frees and NULLs the variable client-options.
oack() - l.1390 - does some stuff, and if an error happens,
client_free() is called with the client structure, which
frees this structure, including client-options, but does
not null it.
So, when returning after an error, client-options is freed
twice, causing a double-free and a crash.
I succeeded in making the server crash, by changing the
tsize and blksize options before transferring a small file
in localhost. In fact, if you look on line 1432, you'll see
that you just have to close the socket to make the server
crash, just after sending the OACK packet.
Typically, I just have to do:
$ tftp localhost
tftp tsize 2
Tsize option on.
tftp blksize 10
tftp get test
[...wait ~ 10 sec...]
tftpd in free(): error: chunk is already free 0x1a658ca65f40
Here is a patch. I switched oack() to int, so that we can
handle errors and call nak() to alert the client before
closing the connection. nak() frees the client structure,
so all goes well.
Ok/Comments?
Index: tftpd.c
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.8
diff -u -r1.8 tftpd.c
--- tftpd.c 13 Jul 2012 02:31:46 - 1.8
+++ tftpd.c 15 Mar 2013 00:36:40 -
@@ -168,7 +168,7 @@
void tftp(struct tftp_client *, struct tftphdr *, size_t);
void tftp_open(struct tftp_client *, const char *);
void nak(struct tftp_client *, int);
-void oack(struct tftp_client *);
+intoack(struct tftp_client *);
void oack_done(int, short, void *);
void sendfile(struct tftp_client *);
@@ -876,8 +876,8 @@
goto error;
if (client-options) {
- oack(client);
-
+ if (oack(client) == -1)
+ goto error;
free(client-options);
client-options = NULL;
} else if (client-opcode == WRQ) {
@@ -1386,7 +1386,7 @@
/*
* Send an oack packet (option acknowledgement).
*/
-void
+int
oack(struct tftp_client *client)
{
struct opt_client *options = client-options;
@@ -1436,10 +1436,10 @@
oack_done, client);
event_add(client-sev, client-tv);
- return;
+ return 0;
error:
- client_free(client);
+ return -1;
}
int
Re: vfs_syscall: doreadlinkat()
Le 24/01/2013 19:10, Matthew Dempsky a écrit : On Thu, Jan 24, 2013 at 9:57 AM, Maxime Villard rusty...@gmx.fr wrote: Hum here, if vp-v_type != VLNK, auio is untouched, but before returning we use auio.uio_resid, which is not initialized. Is it? Nice catch. We should probably move the *retval assignment up just after the VOP_READLINK() call, since this can technically result in undefined behavior if you try to readlink on a non-symlink file. Yes. Index: vfs_syscalls.c === RCS file: /cvs/src/sys/kern/vfs_syscalls.c,v retrieving revision 1.189 diff -u -r1.189 vfs_syscalls.c --- vfs_syscalls.c 10 Sep 2012 11:10:59 - 1.189 +++ vfs_syscalls.c 25 Jan 2013 15:30:30 - @@ -1843,9 +1843,9 @@ auio.uio_procp = p; auio.uio_resid = count; error = VOP_READLINK(vp, auio, p-p_ucred); + *retval = count - auio.uio_resid; } vput(vp); - *retval = count - auio.uio_resid; return (error); } I don't think it should leak any information moment though, since the EINVAL errno will take precedence instead of *retval when we return to userspace.
vfs_syscall: doreadlinkat()
Hi,
I was looking at readlink syscall. There is the following function in
kern/vfs_syscalls.c:
int
doreadlinkat(struct proc *p, int fd, const char *path, char *buf,
size_t count, register_t *retval)
{
struct vnode *vp;
struct iovec aiov;
struct uio auio;
int error;
struct nameidata nd;
NDINITAT(nd, LOOKUP, NOFOLLOW | LOCKLEAF, UIO_USERSPACE, fd, path, p);
if ((error = namei(nd)) != 0)
return (error);
vp = nd.ni_vp;
if (vp-v_type != VLNK)
error = EINVAL;
else {
aiov.iov_base = buf;
aiov.iov_len = count;
auio.uio_iov = aiov;
auio.uio_iovcnt = 1;
auio.uio_offset = 0;
auio.uio_rw = UIO_READ;
auio.uio_segflg = UIO_USERSPACE;
auio.uio_procp = p;
auio.uio_resid = count;
error = VOP_READLINK(vp, auio, p-p_ucred);
}
vput(vp);
*retval = count - auio.uio_resid;
return (error);
}
Hum here, if vp-v_type != VLNK, auio is untouched, but before returning
we use auio.uio_resid, which is not initialized. Is it?
[PATCH] pfctl: leak stuff
Hi,
here are my small changes for pfctl.
1) There are cases where we could leak a file descriptor by
returning.
2) We don't need to check memory before freeing it, as
free() already does that.
3) Just replaced a snprintf() by strlcpy(), it's faster.
Ok/Comments ?
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.314
diff -u -r1.314 pfctl.c
--- pfctl.c 19 Sep 2012 15:52:17 - 1.314
+++ pfctl.c 22 Dec 2012 07:08:28 -
@@ -1377,8 +1377,7 @@
err(1, DIOCXROLLBACK);
exit(1);
} else {/* sub ruleset */
- if (path)
- free(path);
+ free(path);
return (-1);
}
@@ -1867,10 +1866,6 @@
unsigned int len = 0;
size_t n;
- f = fopen(file, w);
- if (f == NULL)
- err(1, open: %s, file);
-
memset(ps, 0, sizeof(ps));
for (;;) {
ps.ps_len = len;
@@ -1893,6 +1888,10 @@
return; /* no states */
len *= 2;
}
+
+ f = fopen(file, w);
+ if (f == NULL)
+ err(1, open: %s, file);
n = ps.ps_len / sizeof(struct pfsync_state);
if (fwrite(inbuf, sizeof(struct pfsync_state), n, f) n)
Index: pfctl_osfp.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_osfp.c,v
retrieving revision 1.18
diff -u -r1.18 pfctl_osfp.c
--- pfctl_osfp.c18 Oct 2010 15:55:28 - 1.18
+++ pfctl_osfp.c22 Dec 2012 07:08:28 -
@@ -112,16 +112,11 @@
while ((line = fgetln(in, len)) != NULL) {
lineno++;
- if (class)
- free(class);
- if (version)
- free(version);
- if (subtype)
- free(subtype);
- if (desc)
- free(desc);
- if (tcpopts)
- free(tcpopts);
+ free(class);
+ free(version);
+ free(subtype);
+ free(desc);
+ free(tcpopts);
class = version = subtype = desc = tcpopts = NULL;
memset(fp, 0, sizeof(fp));
@@ -250,16 +245,11 @@
add_fingerprint(dev, opts, fp);
}
- if (class)
- free(class);
- if (version)
- free(version);
- if (subtype)
- free(subtype);
- if (desc)
- free(desc);
- if (tcpopts)
- free(tcpopts);
+ free(class);
+ free(version);
+ free(subtype);
+ free(desc);
+ free(tcpopts);
fclose(in);
@@ -513,7 +503,7 @@
return (buf);
found:
- snprintf(buf, len, %s, class_name);
+ strlcpy(buf, class_name, len);
if (version_name) {
strlcat(buf, , len);
strlcat(buf, version_name, len);
Index: pfctl_radix.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
retrieving revision 1.29
diff -u -r1.29 pfctl_radix.c
--- pfctl_radix.c 27 Jul 2011 00:26:10 - 1.29
+++ pfctl_radix.c 22 Dec 2012 07:08:28 -
@@ -499,8 +499,7 @@
{
if (b == NULL)
return;
- if (b-pfrb_caddr != NULL)
- free(b-pfrb_caddr);
+ free(b-pfrb_caddr);
b-pfrb_caddr = NULL;
b-pfrb_size = b-pfrb_msize = 0;
}
Re: [PATCH] OpenSSH: auth.c
Le 14/12/2012 06:27, Darren Tucker a écrit :
On Thu, Dec 13, 2012 at 07:31:46PM +0100, Maxime Villard wrote:
Hi,
I was looking at some openssh code when I spotted a mistake
applied, thanks.
Another trivial patch. Make a more detailed error message.
Or, we should use strlcpy().
Ok ?
--- auth.c 2012-12-14 15:54:15.0 +0100
+++ auth.c 2012-12-14 16:22:01.897130976 +0100
@@ -367,7 +367,8 @@
/* for each component of the canonical path, walking upwards */
for (;;) {
if ((cp = dirname(buf)) == NULL) {
- snprintf(err, errlen, dirname() failed);
+ snprintf(err, errlen, dirname %s failed: %s, buf,
+ strerror(errno));
return -1;
}
strlcpy(buf, cp, sizeof(buf));
[PATCH] OpenSSH: auth.c
Hi,
I was looking at some openssh code when I spotted a mistake
in a function from auth.c:
static int
secure_filename(FILE *f, const char *file, struct passwd *pw,
char *err, size_t errlen)
{
char buf[MAXPATHLEN];
struct stat st;
/* check the open file to avoid races */
if (fstat(fileno(f), st) 0) {
snprintf(err, errlen, cannot stat file %s: %s,
buf, strerror(errno));
return -1;
}
return auth_secure_path(file, st, pw-pw_dir, pw-pw_uid, err, errlen);
}
'buf' is not initialized and used whereas it should be 'file'.
Patch:
--- auth.c 2012-12-08 12:51:32.0 +0100
+++ auth.c 2012-12-13 19:11:30.968193729 +0100
@@ -404,13 +404,12 @@
secure_filename(FILE *f, const char *file, struct passwd *pw,
char *err, size_t errlen)
{
- char buf[MAXPATHLEN];
struct stat st;
/* check the open file to avoid races */
if (fstat(fileno(f), st) 0) {
snprintf(err, errlen, cannot stat file %s: %s,
- buf, strerror(errno));
+ file, strerror(errno));
return -1;
}
return auth_secure_path(file, st, pw-pw_dir, pw-pw_uid, err, errlen);
Sendbug getenv
Hi,
== src/usr.bin/sendbug/sendbug.c ==
Tell me if I'm wrong, but in the main() function, we call
getenv() two times (l. 113 134) without holding the result
of the first call.
According to man getenv:
The string pointed to may be overwritten by a subsequent
call to getenv()
After the second call, main() could launch hwdump() which
uses the return value of the first call, which could have been
overwritten by the second one.
We should hold the return value in a char instead of a pointer,
with something like:
--- sendbug.c 2012-07-21 21:55:17.0 +0200
+++ sendbug.c 2012-12-07 19:04:04.770853812 +0100
@@ -83,7 +83,7 @@
{
int ch, c, fd, ret = 1;
struct stat sb;
- char *pr_form;
+ char *pr_form, *tmp;
time_t mtime;
FILE *fp;
@@ -110,7 +110,8 @@
if (argc 0)
usage();
- if ((tmpdir = getenv(TMPDIR)) == NULL || tmpdir[0] == '\0')
+ if ((tmp = getenv(TMPDIR)) == NULL || tmp[0] == '\0' ||
+ (tmpdir = strdup(tmp)) == NULL)
tmpdir = _PATH_TMP;
if (Pflag) {
Shouldn't we ?
