Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Alexander Bluhm 
On Fri, Jan 30, 2015 at 02:34:42PM -0700, Todd C. Miller wrote:
 I think the simplest fix is to just move the m_free to the bad:
 label.

sosetopt() calls m_free() and then it is called again.  So it is a
double free.

I would move the so-so_proto check between the if (name == -1) and
the if (lsa.optlen  MLEN) block.  There m has not been allocated.

Untested as I do not have an i386 right now.

bluhm

Index: sys/compat/linux/linux_socket.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/compat/linux/linux_socket.c,v
retrieving revision 1.59
diff -u -p -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 21:51:05 -
@@ -962,6 +962,14 @@ linux_setsockopt(p, v, retval)
error = EINVAL;
goto bad;
}
+   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;
+   }
if (lsa.optlen  MLEN) {
error = EINVAL;
goto bad;
@@ -974,14 +982,6 @@ linux_setsockopt(p, v, retval)
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:

Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Todd C. Miller 
I think the simplest fix is to just move the m_free to the bad:
label.

 - 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 21:33:38 -
@@ -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;
@@ -985,6 +983,8 @@
}
error = sosetopt(so, level, name, m);
 bad:
+   if (m)
+   (void) m_free(m);
FRELE(fp, p);
return (error);
 }

Re: Allow resuming with closed lid

2015-01-30 Thread Ted Unangst 
On Sat, Jan 31, 2015 at 00:24, Ville Valkonen wrote:
 Hello Mike and Max,
 
 my work laptop is running Windows and on there one must press power button
 to wake up the machine. If I connect the dots right, current behaviour was
 implemented to prevent a hot bag problem. Mimicking the Windows behaviour
 would also prevent laptop wake ups on a bumpy road.
 
 Would it make any sense to mimic this behaviour in obsd?

This obviously depends on your windows config. My windows laptop
resumes whenver I open the lid.

Now what can maybe be improved would be to add a check for which wakeup
event we received. If somebody pushed the power button, the lid
closed check should probably be skipped. If the wakeup event was lid
opening, then making sure the lid really is open makes sense.

Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Maxime Villard 
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

Re: Allow resuming with closed lid

2015-01-30 Thread Ville Valkonen 
Hello Mike and Max,

my work laptop is running Windows and on there one must press power button
to wake up the machine. If I connect the dots right, current behaviour was
implemented to prevent a hot bag problem. Mimicking the Windows behaviour
would also prevent laptop wake ups on a bumpy road.

Would it make any sense to mimic this behaviour in obsd?

--
Kind regards,
Ville

On 30 January 2015 at 05:27, Mike Larkin mlar...@azathoth.net wrote:

 On Fri, Jan 30, 2015 at 12:42:04AM +0100, Max Fillinger wrote:
  Currently, there's code in acpi.c that sends the system back to sleep
  when resuming with closed lid and machdep.lidsuspend=1. I often use my
  laptop in a docking station with an external monitor and keep the lid
  closed, and I'd like to be able to resume just by pushing the power
  button on the docking station. (Also, I first thought something was
  broken when pushing the button only made the suspend-indicator light
  blink for a moment.)
 
  If checking for open lids is necessary in some cases, then I can
  certainly live with lidsuspend=0, but otherwise I'd prefer if it was
  possible to resume with a closed lid. I removed the check and didn't
  notice any problems. The diff below removes the check and also the
  function acpibtn_numopenlids which is not used anywhere else.
 

 This was put in for a reason. I would suggest you go read the commit
 logs and understand why, before proposing reverting functionality
 you obviously have not researched.

 -ml

 
 
  Index: sys/dev/acpi/acpi.c
  ===
  RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
  retrieving revision 1.281
  diff -u -p -r1.281 acpi.c
  --- sys/dev/acpi/acpi.c   17 Jan 2015 04:18:49 -  1.281
  +++ sys/dev/acpi/acpi.c   29 Jan 2015 22:53:42 -
  @@ -2161,7 +2161,6 @@ int
   acpi_sleep_state(struct acpi_softc *sc, int state)
   {
extern int perflevel;
  - extern int lid_suspend;
int error = ENXIO;
int s;
 
  @@ -2305,10 +2304,6 @@ fail_alloc:
 
acpi_record_event(sc, APM_NORMAL_RESUME);
acpi_indicator(sc, ACPI_SST_WORKING);
  -
  - /* If we woke up but all the lids are closed, go back to sleep */
  - if (acpibtn_numopenlids() == 0  lid_suspend != 0)
  - acpi_addtask(sc, acpi_sleep_task, sc, state);
 
   fail_tts:
return (error);
  Index: sys/dev/acpi/acpibtn.c
  ===
  RCS file: /cvs/src/sys/dev/acpi/acpibtn.c,v
  retrieving revision 1.41
  diff -u -p -r1.41 acpibtn.c
  --- sys/dev/acpi/acpibtn.c27 Jan 2015 19:40:14 -  1.41
  +++ sys/dev/acpi/acpibtn.c29 Jan 2015 22:53:42 -
  @@ -74,37 +74,6 @@ struct cfdriver acpibtn_cd = {
 
   const char *acpibtn_hids[] = { ACPI_DEV_LD, ACPI_DEV_PBD, ACPI_DEV_SBD,
 0 };
 
  -/*
  - * acpibtn_numopenlids
  - *
  - * Return the number of _LID devices that are in the open state.
  - * Used to determine if we should go back to sleep/hibernate if we
  - * woke up with the all the lids still closed for some reason. If
  - * the machine has no lids, returns -1.
  - */
  -int
  -acpibtn_numopenlids(void)
  -{
  - struct acpi_lid *lid;
  - int64_t val;
  - int ct = 0;
  -
  - /* If we have no lids ... */
  - if (SLIST_EMPTY(acpibtn_lids))
  - return (-1);
  -
  - /*
  -  * Determine how many lids are open. Assumes _LID evals to
  -  * non-0 or 0, for on / off (which is what the spec says).
  -  */
  - SLIST_FOREACH(lid, acpibtn_lids, abl_link)
  - if (!aml_evalinteger(lid-abl_softc-sc_acpi,
  - lid-abl_softc-sc_devnode, _LID, 0, NULL, val) 
  - val != 0)
  - ct++;
  - return (ct);
  -}
  -
   int
   acpibtn_setpsw(struct acpibtn_softc *sc, int psw)
   {
  Index: sys/dev/acpi/acpidev.h
  ===
  RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
  retrieving revision 1.36
  diff -u -p -r1.36 acpidev.h
  --- sys/dev/acpi/acpidev.h23 Nov 2014 20:33:47 -  1.36
  +++ sys/dev/acpi/acpidev.h29 Jan 2015 22:53:42 -
  @@ -337,5 +337,4 @@ struct acpiec_softc {
 
   void acpibtn_disable_psw(void);
   void acpibtn_enable_psw(void);
  -int  acpibtn_numopenlids(void);
   #endif /* __DEV_ACPI_ACPIDEV_H__ */

Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Ted Unangst 
On Fri, Jan 30, 2015 at 15:57, 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().

So many diffs to choose from! This does retain the original semantics.

Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Alexey Suslikov 
Maxime Villard max at M00nBSD.net writes:

 '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.

compat_linux(8) says:

The Linux compatibility feature is active for kernels compiled with the
COMPAT_LINUX option and kern.emul.linux sysctl(8) enabled.

Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Ted Unangst 
On Fri, Jan 30, 2015 at 22:55, Alexander Bluhm wrote:
 On Fri, Jan 30, 2015 at 02:34:42PM -0700, Todd C. Miller wrote:
 I think the simplest fix is to just move the m_free to the bad:
 label.
 
 sosetopt() calls m_free() and then it is called again.  So it is a
 double free.
 
 I would move the so-so_proto check between the if (name == -1) and
 the if (lsa.optlen  MLEN) block.  There m has not been allocated.
 
 Untested as I do not have an i386 right now.


This will change the sematnics slightly for programs that, for
example, set those options but then pass in an invalid pointer. I
think that's acceptable, however. Well behaved programs will not
notice the difference.

ok

Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Todd C. Miller 
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().

 - 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);
 }

Re: Brainy: User-Triggerable Kernel Memory Leak

2015-01-30 Thread Alexander Bluhm 
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);
  }

syslogd error buffer

2015-01-30 Thread Alexander Bluhm 
Hi,

The error buffer in syslogd might be too small for the TLS errors.
Increase it to 256 bytes and call it ebuf everywhere.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.145
diff -u -p -r1.145 syslogd.c
--- usr.sbin/syslogd/syslogd.c  28 Jan 2015 19:14:05 -  1.145
+++ usr.sbin/syslogd/syslogd.c  30 Jan 2015 23:55:43 -
@@ -727,7 +727,7 @@ int
 tcp_socket(struct filed *f)
 {
int  s, flags;
-   char ebuf[100];
+   char ebuf[256];
 
if ((s = socket(f-f_un.f_forw.f_addr.ss_family, SOCK_STREAM,
IPPROTO_TCP)) == -1) {
@@ -785,7 +785,7 @@ void
 tcp_errorcb(struct bufferevent *bufev, short event, void *arg)
 {
struct filed*f = arg;
-   char ebuf[100];
+   char ebuf[256];
 
if (event  EVBUFFER_EOF)
snprintf(ebuf, sizeof(ebuf),
@@ -882,7 +882,7 @@ struct tls *
 tls_socket(struct filed *f)
 {
struct tls  *ctx;
-   char ebuf[100];
+   char ebuf[256];
 
if ((ctx = tls_client()) == NULL) {
snprintf(ebuf, sizeof(ebuf), tls_client \%s\,
@@ -1425,19 +1425,19 @@ init_signalcb(int signum, short event, v
 void
 logerror(const char *type)
 {
-   char buf[100];
+   char ebuf[256];
 
if (errno)
-   (void)snprintf(buf, sizeof(buf), syslogd: %s: %s,
+   (void)snprintf(ebuf, sizeof(ebuf), syslogd: %s: %s,
type, strerror(errno));
else
-   (void)snprintf(buf, sizeof(buf), syslogd: %s, type);
+   (void)snprintf(ebuf, sizeof(ebuf), syslogd: %s, type);
errno = 0;
-   dprintf(%s\n, buf);
+   dprintf(%s\n, ebuf);
if (Startup)
-   fprintf(stderr, %s\n, buf);
+   fprintf(stderr, %s\n, ebuf);
else
-   logmsg(LOG_SYSLOG|LOG_ERR, buf, LocalHostName, ADDDATE);
+   logmsg(LOG_SYSLOG|LOG_ERR, ebuf, LocalHostName, ADDDATE);
 }
 
 void
@@ -1445,7 +1445,7 @@ die(int signo)
 {
struct filed *f;
int was_initialized = Initialized;
-   char buf[100];
+   char ebuf[256];
 
Initialized = 0;/* Don't log SIGCHLDs */
SIMPLEQ_FOREACH(f, Files, f_next) {
@@ -1456,9 +1456,10 @@ die(int signo)
Initialized = was_initialized;
if (signo) {
dprintf(syslogd: exiting on signal %d\n, signo);
-   (void)snprintf(buf, sizeof buf, exiting on signal %d, signo);
+   (void)snprintf(ebuf, sizeof(ebuf), exiting on signal %d,
+   signo);
errno = 0;
-   logerror(buf);
+   logerror(ebuf);
}
dprintf([unpriv] syslogd child about to exit\n);
exit(0);
@@ -1708,7 +1709,7 @@ cfline(char *line, char *prog)
int i, pri;
size_t rb_len;
char *bp, *p, *q, *proto, *host, *port, *ipproto;
-   char buf[MAXLINE], ebuf[100];
+   char buf[MAXLINE], ebuf[256];
struct filed *xf, *f, *d;
struct timeval to;
 
@@ -2118,7 +2119,7 @@ int
 unix_socket(char *path, int type, mode_t mode)
 {
struct sockaddr_un s_un;
-   char errbuf[512];
+   char ebuf[512];
int fd;
mode_t old_umask;
 
@@ -2126,9 +2127,8 @@ unix_socket(char *path, int type, mode_t
s_un.sun_family = AF_UNIX;
if (strlcpy(s_un.sun_path, path, sizeof(s_un.sun_path)) =
sizeof(s_un.sun_path)) {
-   snprintf(errbuf, sizeof(errbuf), socket path too long: %s,
-   path);
-   logerror(errbuf);
+   snprintf(ebuf, sizeof(ebuf), socket path too long: %s, path);
+   logerror(ebuf);
die(0);
}
 
@@ -2151,8 +2151,8 @@ unix_socket(char *path, int type, mode_t
 
unlink(path);
if (bind(fd, (struct sockaddr *)s_un, SUN_LEN(s_un)) == -1) {
-   snprintf(errbuf, sizeof(errbuf), cannot bind %s, path);
-   logerror(errbuf);
+   snprintf(ebuf, sizeof(ebuf), cannot bind %s, path);
+   logerror(ebuf);
umask(old_umask);
close(fd);
return (-1);
@@ -2161,8 +2161,8 @@ unix_socket(char *path, int type, mode_t
umask(old_umask);
 
if (chmod(path, mode) == -1) {
-   snprintf(errbuf, sizeof(errbuf), cannot chmod %s, path);
-   logerror(errbuf);
+   snprintf(ebuf, sizeof(ebuf), cannot chmod %s, path);
+   logerror(ebuf);
close(fd);
unlink(path);
return (-1);

Re: elantech-v4 clickpad support

2015-01-30 Thread Ulf Brosziewski 

On 01/30/2015 07:15 AM, Martin Pieuchot wrote:

On 30/01/15(Fri) 01:25, Ulf Brosziewski wrote:

Probably I was too sceptical about synaptics.c. The bug I observed
with the ALPS touchpad seems to be due to a kind of mismatch between
the ALPS code in pms and the event handling in wsconscomm. The patch
below contains the initial change as well as what was necessary to
fix this.


Do you think it is possible to fix the pms(4) driver instead of adding
another quirk?


 ...


Certainly that would be a better solution. For synaptics hardware there
seems to be no specific W value that signals the end of a touch. If I
understand it correctly, the hardware reports zero coordinates instead
and the X driver adjusts its state accordingly. I will try to check soon
whether this is correct and whether the ALPS code could be adapted.