* [PATCH] tty/vt: set_get_cmap() check user buffer
@ 2012-03-19 23:34 Michael Gehring
2012-03-19 23:48 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Michael Gehring @ 2012-03-19 23:34 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel, Michael Gehring
set_get_cmap() ignores the result of {get,put}_user(), causing ioctl(vt,
{G,P}IO_CMAP, 0xdeadbeef) to silently fail.
Another side effect of this: calling the PIO_CMAP ioctl with an invalid
buffer will zero the default colormap and the palette for all vts (all
colors set to black).
Use access_ok() and return -EFAULT when appropriate.
Signed-off-by: Michael Gehring <mg@ebfe.org>
---
drivers/tty/vt/vt.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index e716839..176b2a1 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)
WARN_CONSOLE_UNLOCKED();
+ if (!access_ok(set ? VERIFY_READ : VERIFY_WRITE, arg, 3 * 16))
+ return -EFAULT;
+
for (i = 0; i < 16; i++)
if (set) {
- get_user(default_red[i], arg++);
- get_user(default_grn[i], arg++);
- get_user(default_blu[i], arg++);
+ __get_user(default_red[i], arg++);
+ __get_user(default_grn[i], arg++);
+ __get_user(default_blu[i], arg++);
} else {
- put_user(default_red[i], arg++);
- put_user(default_grn[i], arg++);
- put_user(default_blu[i], arg++);
+ __put_user(default_red[i], arg++);
+ __put_user(default_grn[i], arg++);
+ __put_user(default_blu[i], arg++);
}
if (set) {
for (i = 0; i < MAX_NR_CONSOLES; i++)
--
1.7.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tty/vt: set_get_cmap() check user buffer
2012-03-19 23:34 [PATCH] tty/vt: set_get_cmap() check user buffer Michael Gehring
@ 2012-03-19 23:48 ` Greg Kroah-Hartman
2012-03-20 1:07 ` Michael Gehring
2012-03-21 0:26 ` [PATCH] tty/vt: handle bad user buffer in {G,P}IO_CMAP ioctl Michael Gehring
0 siblings, 2 replies; 6+ messages in thread
From: Greg Kroah-Hartman @ 2012-03-19 23:48 UTC (permalink / raw)
To: Michael Gehring; +Cc: linux-kernel
On Tue, Mar 20, 2012 at 12:34:01AM +0100, Michael Gehring wrote:
> set_get_cmap() ignores the result of {get,put}_user(), causing ioctl(vt,
> {G,P}IO_CMAP, 0xdeadbeef) to silently fail.
Why not just check each return value, failing only if/when a specific
write fails?
>
> Another side effect of this: calling the PIO_CMAP ioctl with an invalid
> buffer will zero the default colormap and the palette for all vts (all
> colors set to black).
>
> Use access_ok() and return -EFAULT when appropriate.
>
> Signed-off-by: Michael Gehring <mg@ebfe.org>
> ---
> drivers/tty/vt/vt.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index e716839..176b2a1 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)
>
> WARN_CONSOLE_UNLOCKED();
>
> + if (!access_ok(set ? VERIFY_READ : VERIFY_WRITE, arg, 3 * 16))
> + return -EFAULT;
> +
> for (i = 0; i < 16; i++)
> if (set) {
> - get_user(default_red[i], arg++);
> - get_user(default_grn[i], arg++);
> - get_user(default_blu[i], arg++);
> + __get_user(default_red[i], arg++);
> + __get_user(default_grn[i], arg++);
> + __get_user(default_blu[i], arg++);
> } else {
> - put_user(default_red[i], arg++);
> - put_user(default_grn[i], arg++);
> - put_user(default_blu[i], arg++);
> + __put_user(default_red[i], arg++);
> + __put_user(default_grn[i], arg++);
> + __put_user(default_blu[i], arg++);
What's to keep this userspace buffer from becoming invalid after the
check? For some reason I thought we couldn't check beforehand like
this, but I can't recall why at this specific moment.
And ugh, why do we have a function that does two things, like this? The
only thing we are "saving" is a single for loop by doing things this
way, splitting it out into a set/get function, would make more sense in
the end.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty/vt: set_get_cmap() check user buffer
2012-03-19 23:48 ` Greg Kroah-Hartman
@ 2012-03-20 1:07 ` Michael Gehring
2012-03-20 8:38 ` Jiri Slaby
2012-03-20 23:00 ` Alan Cox
2012-03-21 0:26 ` [PATCH] tty/vt: handle bad user buffer in {G,P}IO_CMAP ioctl Michael Gehring
1 sibling, 2 replies; 6+ messages in thread
From: Michael Gehring @ 2012-03-20 1:07 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-kernel
On Mon, Mar 19, 2012 at 04:48:39PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Mar 20, 2012 at 12:34:01AM +0100, Michael Gehring wrote:
> > set_get_cmap() ignores the result of {get,put}_user(), causing ioctl(vt,
> > {G,P}IO_CMAP, 0xdeadbeef) to silently fail.
>
> Why not just check each return value, failing only if/when a specific
> write fails?
For the 'set' case, that would result in a partially updated default
colormap.
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index e716839..176b2a1 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)
...
> What's to keep this userspace buffer from becoming invalid after the
> check? For some reason I thought we couldn't check beforehand like
> this, but I can't recall why at this specific moment.
>
> And ugh, why do we have a function that does two things, like this? The
> only thing we are "saving" is a single for loop by doing things this
> way, splitting it out into a set/get function, would make more sense in
> the end.
How about something like this (untested):
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index e716839..f0a881d 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3928,24 +3928,53 @@ static int set_get_cmap(unsigned char __user *arg, int set)
int con_set_cmap(unsigned char __user *arg)
{
- int rc;
+ int i, j, k;
+ unsigned char colormap[3*16];
+
+ if (copy_from_user(colormap, arg, sizeof(colormap)))
+ return -EFAULT;
console_lock();
- rc = set_get_cmap (arg,1);
+
+ for (i = k = 0; i < 16; i++) {
+ default_red[i] = colormap[k++];
+ default_grn[i] = colormap[k++];
+ default_blu[i] = colormap[k++];
+ }
+
+ for (i = 0; i < MAX_NR_CONSOLES; i++) {
+ if (!vc_cons_allocated(i))
+ continue;
+ for (j = k = 0; j < 16; j++) {
+ vc_cons[i].d->vc_palette[k++] = default_red[j];
+ vc_cons[i].d->vc_palette[k++] = default_grn[j];
+ vc_cons[i].d->vc_palette[k++] = default_blu[j];
+ }
+ set_palette(vc_cons[i].d);
+ }
+
console_unlock();
- return rc;
+ return 0;
}
int con_get_cmap(unsigned char __user *arg)
{
- int rc;
+ int i, k;
+ unsigned char colormap[3*16];
console_lock();
- rc = set_get_cmap (arg,0);
+ for (i = k = 0; i < 16; i++) {
+ colormap[k++] = default_red[i];
+ colormap[k++] = default_grn[i];
+ colormap[k++] = default_blu[i];
+ }
console_unlock();
- return rc;
+ if (copy_to_user(arg, colormap, sizeof(colormap)))
+ return -EFAULT;
+
+ return 0;
}
void reset_palette(struct vc_data *vc)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tty/vt: set_get_cmap() check user buffer
2012-03-20 1:07 ` Michael Gehring
@ 2012-03-20 8:38 ` Jiri Slaby
2012-03-20 23:00 ` Alan Cox
1 sibling, 0 replies; 6+ messages in thread
From: Jiri Slaby @ 2012-03-20 8:38 UTC (permalink / raw)
To: Michael Gehring; +Cc: Greg Kroah-Hartman, linux-kernel
On 03/20/2012 02:07 AM, Michael Gehring wrote:
> On Mon, Mar 19, 2012 at 04:48:39PM -0700, Greg Kroah-Hartman wrote:
>> On Tue, Mar 20, 2012 at 12:34:01AM +0100, Michael Gehring wrote:
>>> set_get_cmap() ignores the result of {get,put}_user(), causing ioctl(vt,
>>> {G,P}IO_CMAP, 0xdeadbeef) to silently fail.
>>
>> Why not just check each return value, failing only if/when a specific
>> write fails?
>
> For the 'set' case, that would result in a partially updated default
> colormap.
>
>>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>>> index e716839..176b2a1 100644
>>> --- a/drivers/tty/vt/vt.c
>>> +++ b/drivers/tty/vt/vt.c
>>> @@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)
> ...
>> What's to keep this userspace buffer from becoming invalid after the
>> check? For some reason I thought we couldn't check beforehand like
>> this, but I can't recall why at this specific moment.
>>
>> And ugh, why do we have a function that does two things, like this? The
>> only thing we are "saving" is a single for loop by doing things this
>> way, splitting it out into a set/get function, would make more sense in
>> the end.
>
>
> How about something like this (untested):
FWIW it very much makes sense to me.
Except you should also remove set_get_cmap now.
And make sure it really works (test it).
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index e716839..f0a881d 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3928,24 +3928,53 @@ static int set_get_cmap(unsigned char __user *arg, int set)
>
> int con_set_cmap(unsigned char __user *arg)
> {
> - int rc;
> + int i, j, k;
> + unsigned char colormap[3*16];
> +
> + if (copy_from_user(colormap, arg, sizeof(colormap)))
> + return -EFAULT;
>
> console_lock();
> - rc = set_get_cmap (arg,1);
> +
> + for (i = k = 0; i < 16; i++) {
> + default_red[i] = colormap[k++];
> + default_grn[i] = colormap[k++];
> + default_blu[i] = colormap[k++];
> + }
> +
> + for (i = 0; i < MAX_NR_CONSOLES; i++) {
> + if (!vc_cons_allocated(i))
> + continue;
> + for (j = k = 0; j < 16; j++) {
> + vc_cons[i].d->vc_palette[k++] = default_red[j];
> + vc_cons[i].d->vc_palette[k++] = default_grn[j];
> + vc_cons[i].d->vc_palette[k++] = default_blu[j];
> + }
> + set_palette(vc_cons[i].d);
> + }
> +
> console_unlock();
>
> - return rc;
> + return 0;
> }
>
> int con_get_cmap(unsigned char __user *arg)
> {
> - int rc;
> + int i, k;
> + unsigned char colormap[3*16];
>
> console_lock();
> - rc = set_get_cmap (arg,0);
> + for (i = k = 0; i < 16; i++) {
> + colormap[k++] = default_red[i];
> + colormap[k++] = default_grn[i];
> + colormap[k++] = default_blu[i];
> + }
> console_unlock();
>
> - return rc;
> + if (copy_to_user(arg, colormap, sizeof(colormap)))
> + return -EFAULT;
> +
> + return 0;
> }
>
> void reset_palette(struct vc_data *vc)
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tty/vt: set_get_cmap() check user buffer
2012-03-20 1:07 ` Michael Gehring
2012-03-20 8:38 ` Jiri Slaby
@ 2012-03-20 23:00 ` Alan Cox
1 sibling, 0 replies; 6+ messages in thread
From: Alan Cox @ 2012-03-20 23:00 UTC (permalink / raw)
To: Michael Gehring; +Cc: Greg Kroah-Hartman, linux-kernel
> For the 'set' case, that would result in a partially updated default
> colormap.
Agreed - I don't think it really matters much but its so trivial to
do right.
This looks the right model to me.
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] tty/vt: handle bad user buffer in {G,P}IO_CMAP ioctl
2012-03-19 23:48 ` Greg Kroah-Hartman
2012-03-20 1:07 ` Michael Gehring
@ 2012-03-21 0:26 ` Michael Gehring
1 sibling, 0 replies; 6+ messages in thread
From: Michael Gehring @ 2012-03-21 0:26 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, Alan Cox, linux-kernel, Michael Gehring
set_get_cmap() ignored the result of {get,put}_user(), causing ioctl(vt,
{G,P}IO_CMAP, 0xdeadbeef) to silently fail.
Another side effect of this: calling the PIO_CMAP ioctl with an invalid
buffer would zero the default colormap and the palette for all vts (all
colors set to black).
Leave the default colormap intact and return -EFAULT when
reading/writing to the userspace buffer fails.
Signed-off-by: Michael Gehring <mg@ebfe.org>
---
drivers/tty/vt/vt.c | 68 ++++++++++++++++++++++++---------------------------
1 file changed, 32 insertions(+), 36 deletions(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index e716839..49c2dec 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3891,36 +3891,6 @@ static void set_palette(struct vc_data *vc)
vc->vc_sw->con_set_palette(vc, color_table);
}
-static int set_get_cmap(unsigned char __user *arg, int set)
-{
- int i, j, k;
-
- WARN_CONSOLE_UNLOCKED();
-
- for (i = 0; i < 16; i++)
- if (set) {
- get_user(default_red[i], arg++);
- get_user(default_grn[i], arg++);
- get_user(default_blu[i], arg++);
- } else {
- put_user(default_red[i], arg++);
- put_user(default_grn[i], arg++);
- put_user(default_blu[i], arg++);
- }
- if (set) {
- for (i = 0; i < MAX_NR_CONSOLES; i++)
- if (vc_cons_allocated(i)) {
- for (j = k = 0; j < 16; j++) {
- vc_cons[i].d->vc_palette[k++] = default_red[j];
- vc_cons[i].d->vc_palette[k++] = default_grn[j];
- vc_cons[i].d->vc_palette[k++] = default_blu[j];
- }
- set_palette(vc_cons[i].d);
- }
- }
- return 0;
-}
-
/*
* Load palette into the DAC registers. arg points to a colour
* map, 3 bytes per colour, 16 colours, range from 0 to 255.
@@ -3928,24 +3898,50 @@ static int set_get_cmap(unsigned char __user *arg, int set)
int con_set_cmap(unsigned char __user *arg)
{
- int rc;
+ int i, j, k;
+ unsigned char colormap[3*16];
+
+ if (copy_from_user(colormap, arg, sizeof(colormap)))
+ return -EFAULT;
console_lock();
- rc = set_get_cmap (arg,1);
+ for (i = k = 0; i < 16; i++) {
+ default_red[i] = colormap[k++];
+ default_grn[i] = colormap[k++];
+ default_blu[i] = colormap[k++];
+ }
+ for (i = 0; i < MAX_NR_CONSOLES; i++) {
+ if (!vc_cons_allocated(i))
+ continue;
+ for (j = k = 0; j < 16; j++) {
+ vc_cons[i].d->vc_palette[k++] = default_red[j];
+ vc_cons[i].d->vc_palette[k++] = default_grn[j];
+ vc_cons[i].d->vc_palette[k++] = default_blu[j];
+ }
+ set_palette(vc_cons[i].d);
+ }
console_unlock();
- return rc;
+ return 0;
}
int con_get_cmap(unsigned char __user *arg)
{
- int rc;
+ int i, k;
+ unsigned char colormap[3*16];
console_lock();
- rc = set_get_cmap (arg,0);
+ for (i = k = 0; i < 16; i++) {
+ colormap[k++] = default_red[i];
+ colormap[k++] = default_grn[i];
+ colormap[k++] = default_blu[i];
+ }
console_unlock();
- return rc;
+ if (copy_to_user(arg, colormap, sizeof(colormap)))
+ return -EFAULT;
+
+ return 0;
}
void reset_palette(struct vc_data *vc)
--
1.7.9.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-21 0:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-19 23:34 [PATCH] tty/vt: set_get_cmap() check user buffer Michael Gehring
2012-03-19 23:48 ` Greg Kroah-Hartman
2012-03-20 1:07 ` Michael Gehring
2012-03-20 8:38 ` Jiri Slaby
2012-03-20 23:00 ` Alan Cox
2012-03-21 0:26 ` [PATCH] tty/vt: handle bad user buffer in {G,P}IO_CMAP ioctl Michael Gehring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).