Alsa-Devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>,
	Eliot Blennerhassett <eliot@blennerhassett.gen.nz>,
	alsa-devel@alsa-project.org, kernel-janitors@vger.kernel.org
Subject: [patch] ALSA: asihpi: fix an information leak in asihpi_hpi_ioctl()
Date: Mon, 22 Dec 2014 10:49:46 +0300	[thread overview]
Message-ID: <20141222074946.GA9737@mwanda> (raw)

So far as I can see "hr->h.size" is set to "res_max_size" which is a
user controlled value between 12 and USHRT_MAX.  If it's larger than
sizeof(*hr), then that leads to an information leak.

I am not very familiar with this code, my other question here is that
on lines before we set "hr->h.size = sizeof(hr->h)".  It think this is
a bug.  I also think this particular code is never executed and I added
a comment to that effect.  But we do it in earlier in the function as
well:

	copy_to_user(puhr, hr, sizeof(hr->h));

It doesn't make sense to me.

Anyway, I think my patch is safe and it seems to fix a real information
leak.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/sound/pci/asihpi/hpioctl.c b/sound/pci/asihpi/hpioctl.c
index 6aa677e..f88109a 100644
--- a/sound/pci/asihpi/hpioctl.c
+++ b/sound/pci/asihpi/hpioctl.c
@@ -283,6 +283,7 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		goto out;
 	}
 
+	/* FIXME:  isn't this a no-op? */
 	if (hr->h.size > res_max_size) {
 		HPI_DEBUG_LOG(ERROR, "response too big %d %d\n", hr->h.size,
 			res_max_size);
@@ -290,6 +291,9 @@ long asihpi_hpi_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		hr->h.specific_error = hr->h.size;
 		hr->h.size = sizeof(hr->h);
 	}
+	/* prevent an information leak */
+	if (hr->h.size > sizeof(*hr))
+		hr->h.size = sizeof(*hr);
 
 	uncopied_bytes = copy_to_user(puhr, hr, hr->h.size);
 	if (uncopied_bytes) {

             reply	other threads:[~2014-12-22  7:49 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-22  7:49 Dan Carpenter [this message]
2014-12-26 11:07 ` [patch] ALSA: asihpi: fix an information leak in asihpi_hpi_ioctl() Takashi Iwai
2014-12-30  8:07   ` Eliot Blennerhassett
2014-12-31  6:26     ` Eliot Blennerhassett
2014-12-31  8:40       ` Takashi Iwai
     [not found]         ` <54A3D480.8050104@blennerhassett.gen.nz>
2014-12-31 13:17           ` [patch v2] " Takashi Iwai
2015-01-05  9:33       ` [patch] " Dan Carpenter
2015-01-05  9:34         ` Takashi Iwai

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141222074946.GA9737@mwanda \
    --to=dan.carpenter@oracle.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=eliot@blennerhassett.gen.nz \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox