CVE-2013-1798: Linux kernel KVM IOAPIC_REG_SELECT Invalid Memory Access

This was very nice vulnerability reported by Andrew Honig of Google. The bug is triggered when a user specifies an invalid IOAPIC_REG_SELECT value which is reachable via read KVM I/O device operation as you can see below.

static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
			    void *val)
{
	struct kvm_ioapic *ioapic = to_ioapic(this);
	u32 result;
   ...
	switch (addr) {
	case IOAPIC_REG_SELECT:
		result = ioapic->ioregsel;
		break;

	case IOAPIC_REG_WINDOW:
		result = ioapic_read_indirect(ioapic, addr, len);
		break;
   ...
	return 0;
}
   ...
static const struct kvm_io_device_ops ioapic_mmio_ops = {
	.read     = ioapic_mmio_read,
	.write    = ioapic_mmio_write,
};

Additionally, if a user makes a read by invoking IOAPIC_REG_WINDOW it will result in calling ioapic_read_indirect(). Here is what this function does.

static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
					  unsigned long addr,
					  unsigned long length)
{
	unsigned long result = 0;

	switch (ioapic->ioregsel) {
  ...
	default:
		{
			u32 redir_index = (ioapic->ioregsel - 0x10) >> 1;
			u64 redir_content;

			ASSERT(redir_index < IOAPIC_NUM_PINS);

			redir_content = ioapic->redirtbl[redir_index].bits;
			result = (ioapic->ioregsel & 0x1) ?
			    (redir_content >> 32) & 0xffffffff :
			    redir_content & 0xffffffff;
			break;
		}
	}

	return result;
}

It calculates and initializes the value of ‘redir_index’ from the user controlled ‘ioapic->ioregsel’ variable and then uses it as an index to ‘ioapic->redirtbl[]‘ array. If this value is larger than IOAPIC_NUM_PINS it will result in invalid memory access. Here is how IOAPIC_NUM_PINS is defined in virt/kvm/ioapic.h header file.

#define IOAPIC_NUM_PINS  KVM_IOAPIC_NUM_PINS

And this is because it is architecture specific. For IA64 is defined in include/uapi/asm/kvm.h as 48 and for x86 in arch/x86/include/uapi/asm/kvm.h as 24. As you might have noticed there is an ASSERT() call to make this check but of course, this will only take effect in the debug builds.
The fix was to replace that ASSERT() call with a range check like this.

 			u32 redir_index = (ioapic->ioregsel - 0x10) >> 1;
 			u64 redir_content;
-			ASSERT(redir_index < IOAPIC_NUM_PINS);
+			if (redir_index < IOAPIC_NUM_PINS)
+				redir_content =
+					ioapic->redirtbl[redir_index].bits;
+			else
+				redir_content = ~0ULL;
-			redir_content = ioapic->redirtbl[redir_index].bits;
 			result = (ioapic->ioregsel & 0x1) ?
 			    (redir_content >> 32) & 0xffffffff :

CVE-2013-2074: KDE kdelibs Password Exposure

This was a low impact vulnerability reported by m.wege. The issue occurs on “internal server error” and triggers the below code located at kioslave/http/http.cpp.

/**
 * This function will read in the return header from the server.  It will
 * not read in the body of the return message.  It will also not transmit
 * the header to our client as the client doesn't need to know the gory
 * details of HTTP headers.
 */
bool HTTPProtocol::readResponseHeader()
{
   ...
    if (m_request.responseCode >= 500 && m_request.responseCode <= 599) {
        // Server side errors

        if (m_request.method == HTTP_HEAD) {
   ...
            if (!sendErrorPageNotification()) {
                error(ERR_INTERNAL_SERVER, m_request.url.url());
                return false;
   ...
    } else if (!isAuthenticationRequired(m_request.responseCode) && m_request.responseCode >= 400 && m_request.responseCode <= 499) {
        // Any other client errors
        // Tell that we will only get an error page here.
        if (!sendErrorPageNotification()) {
            if (m_request.responseCode == 403)
                error(ERR_ACCESS_DENIED, m_request.url.url());
            else
                error(ERR_DOES_NOT_EXIST, m_request.url.url());
            return false;
        }
   ...
}

Clearly, error() could be called passing the error number along with a string which in all of the above cases is the result of m_request.url.url().

void HTTPProtocol::error( int _err, const QString &_text )
{
  // Close the connection only on connection errors. Otherwise, honor the
  // keep alive flag.
  if (_err == ERR_CONNECTION_BROKEN || _err == ERR_COULD_NOT_CONNECT)
      httpClose(false);
  else
      httpClose(m_request.isKeepAlive);

  if (!m_request.id.isEmpty())
  {
    forwardHttpResponseHeader();
    sendMetaData();
  }

  // It's over, we don't need it anymore
  clearPostDataBuffer();

  SlaveBase::error( _err, _text );
  m_iError = _err;
}

The url() routine is part of kdecore/io/kurl.cpp file and it is being to used to dump the URL.

QString KUrl::url( AdjustPathOption trailing ) const
{
  if (QString::compare(scheme(), QLatin1String("mailto"), Qt::CaseInsensitive) == 0) {
      // mailto urls should be prettified, see the url183433 testcase.
      return prettyUrl(trailing);
  }
  if ( trailing == AddTrailingSlash && !path().endsWith( QLatin1Char('/') ) ) {
      // -1 and 0 are provided by QUrl, but not +1, so that one is a bit tricky.
      // To avoid reimplementing toEncoded() all over again, I just use another QUrl
      // Let's hope this is fast, or not called often...
      QUrl newUrl( *this );
      newUrl.setPath( path() + QLatin1Char('/') );
      return QString::fromLatin1(newUrl.toEncoded());
  }
  else if ( trailing == RemoveTrailingSlash) {
      const QString cleanedPath = trailingSlash(trailing, path());
      if (cleanedPath == QLatin1String("/")) {
          if (path() != QLatin1String("/")) {
              QUrl fixedUrl = *this;
              fixedUrl.setPath(cleanedPath);
              return QLatin1String(fixedUrl.toEncoded(None));
          }
          return QLatin1String(toEncoded(None));
      }
  }
  return QString::fromLatin1(toEncoded(trailing == RemoveTrailingSlash ? StripTrailingSlash : None));
}

Of course, this means that the same also applies when the URL includes username and password information such as http://username:password@example.com. To fix this the calls to url() were replaced by prettyUrl() with the following patch.

diff --git a/kioslave/http/http.cpp b/kioslave/http/http.cpp
index 2d139a9..129fc7b 100644
--- a/kioslave/http/http.cpp
+++ b/kioslave/http/http.cpp
@@ -3056,7 +3056,7 @@ try_again:
             ; // Ignore error
         } else {
             if (!sendErrorPageNotification()) {
-                error(ERR_INTERNAL_SERVER, m_request.url.url());
+                error(ERR_INTERNAL_SERVER, m_request.url.prettyUrl());
                 return false;
             }
         }
@@ -3072,9 +3072,9 @@ try_again:
         // Tell that we will only get an error page here.
         if (!sendErrorPageNotification()) {
             if (m_request.responseCode == 403)
-                error(ERR_ACCESS_DENIED, m_request.url.url());
+                error(ERR_ACCESS_DENIED, m_request.url.prettyUrl());
             else
-                error(ERR_DOES_NOT_EXIST, m_request.url.url());
+                error(ERR_DOES_NOT_EXIST, m_request.url.prettyUrl());
             return false;
         }
     } else if (m_request.responseCode >= 301 && m_request.responseCode<= 303) {

This routine among other things removes the password from the URL.

QString KUrl::prettyUrl( AdjustPathOption trailing ) const
{
  // reconstruct the URL in a "pretty" form
  // a "pretty" URL is NOT suitable for data transfer. It's only for showing data to the user.
  // however, it must be parseable back to its original state, since
  // notably Konqueror displays it in the Location address.

  // A pretty URL is the same as a normal URL, except that:
  // - the password is removed
  // - the hostname is shown in Unicode (as opposed to ACE/Punycode)
  // - the pathname and fragment parts are shown in Unicode (as opposed to %-encoding)
  QString result = scheme();
    ...
}

CVE-2013-1796: Linux kernel KVM MSR_KVM_SYSTEM_TIME Buffer Overflow

This is a really nice vulnerability killed by Andy Honig. It is particularly interesting because it allows host kernel memory corruption through guest GPA (Guest Physical Address) manipulation. If we have a look in arch/x86/kvm/x86.c we can see the following code.

int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
{
	bool pr = false;
	u32 msr = msr_info->index;
	u64 data = msr_info->data;

	switch (msr) {   
   ...
	case MSR_KVM_SYSTEM_TIME: {
		kvmclock_reset(vcpu);

		vcpu->arch.time = data;
		kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);

		/* we verify if the enable bit is set... */
		if (!(data & 1))
			break;

		/* ...but clean it before doing the actual write */
		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);

		vcpu->arch.time_page =
				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);

		if (is_error_page(vcpu->arch.time_page))
			vcpu->arch.time_page = NULL;

		break;
	}
   ...
	return 0;
}
EXPORT_SYMBOL_GPL(kvm_set_msr_common);

So by utilizing the ‘MSR_KVM_SYSTEM_TIME’ kvmclock MSR a user can set ‘vcpu->arch.time_page’ through gfn_to_page() call that uses the user derived ‘data’ information. As Andy Honig mentioned in his commit, the arbitrary write occurs when kmap atomic attempts to obtain a pointer to the time structure page and performing a memcpy() to it starting at the user controlled offset. The fix was to add a check that verifies that the provided value does not exceed the structure’s boundaries.

 		/* ...but clean it before doing the actual write */
 		vcpu->arch.time_offset = data & ~(PAGE_MASK | 1);
+		/* Check that the address is 32-byte aligned. */
+		if (vcpu->arch.time_offset &
+				(sizeof(struct pvclock_vcpu_time_info) - 1))
+			break;
+
 		vcpu->arch.time_page =
 				gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT);

CVE-2013-1848: Linux kernel EXT3 ext3_msg() Format String

Recently Lars-Peter Clausen committed a change on Linux kernel that fixes a format string vulnerability in the EXT3 filesystem code. The susceptible code resides in fs/ext3/super.c but to better understand it we need to have a look on how ext3_msg() is defined first.

void ext3_msg(struct super_block *sb, const char *prefix,
                const char *fmt, ...)
{
        struct va_format vaf;
        va_list args;
 
        va_start(args, fmt);
 
        vaf.fmt = fmt;
        vaf.va = &args;
 
        printk("%sEXT3-fs (%s): %pV\n", prefix, sb->s_id, &vaf);
 
        va_end(args);
}

So, it should be called passing the following three mandatory arguments:
- Pointer to the super-block structure
- Prefix string
- Format string
And of course, any variables to be printed. As Lars-Peter Clausen noticed, there were two cases where there was no prefix defined. This makes the format string argument to be passed as prefix and any variables to be processed as the format string. Here are these two cases:

/*
 * Open the external journal device
 */
static struct block_device *ext3_blkdev_get(dev_t dev, struct super_block *sb)
{
  ...
fail:
        ext3_msg(sb, "error: failed to open journal device %s: %ld",
                __bdevname(dev, b), PTR_ERR(bdev));

        return NULL;
}

And…

static ext3_fsblk_t get_sb_block(void **data, struct super_block *sb)
{
        ext3_fsblk_t    sb_block;
  ...
        if (*options && *options != ',') {
                ext3_msg(sb, "error: invalid sb specification: %s",
                       (char *) *data);
  ...
        return sb_block;
}

The fix was to add the missing prefix argument to the function call like this.

@@ -353,7 +353,7 @@ static struct block_device *ext3_blkdev_get(dev_t dev, struct super_block *sb)
 	return bdev;
 fail:
-	ext3_msg(sb, "error: failed to open journal device %s: %ld",
+	ext3_msg(sb, KERN_ERR, "error: failed to open journal device %s: %ld",
 		__bdevname(dev, b), PTR_ERR(bdev));
 	return NULL;
@@ -887,7 +887,7 @@ static ext3_fsblk_t get_sb_block(void **data, struct super_block *sb)
 	/*todo: use simple_strtoll with >32bit ext3 */
 	sb_block = simple_strtoul(options, &options, 0);
 	if (*options && *options != ',') {
-		ext3_msg(sb, "error: invalid sb specification: %s",
+		ext3_msg(sb, KERN_ERR, "error: invalid sb specification: %s",
 		       (char *) *data);
 		return 1;
 	}

CVE-2013-1774: Linux kernel Edgeport USB Serial Converter NULL Pointer Dereference

This is a vulnerability fixed by Wolfgang Frisch and the buggy code resides in drivers/usb/serial/io_ti.c as you can see below.

static void chase_port(struct edgeport_port *port, unsigned long timeout,
								int flush)
{
	int baud_rate;
	struct tty_struct *tty = tty_port_tty_get(&port->port->port);
	struct usb_serial *serial = port->port->serial;
	wait_queue_t wait;
	unsigned long flags;
   ...
	remove_wait_queue(&tty->write_wait, &wait);
   ...
	tty_kref_put(tty);
   ...
}

If the equivalent /dev/ttyUSB device file is in use while the device is disconnected then any call to chase_port() (used to chase the port, close and flush it) will lead to NULL pointer dereference since there is no longer a ‘tty’ associated with it. The fix was to add a simple check for this case.

	unsigned long flags;
+	if (!tty)
+		return;
+
	if (!timeout)