* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.