tty/vt: set_get_cmap() check user buffer
PATCH 1/1 tty/vt: set_get_cmap() check user buffer
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.
--- drivers/tty/vt/vt.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
drivers/tty/vt/vt.c +9 -6
@@ -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++)
Signature · 1 line
1.7.9.4
Greg Kroah-Hartman
maintainerQuoted 2 lines · Michael Gehring
set_get_cmap()ignores the result of {get,put}_user(), causingioctl(vt, {G,P}IO_CMAP, 0xdeadbeef)to silently fail.
Why not just check each return value, failing only if/when a specific write fails?
Quoted 28 lines
Another side effect of this: calling the
PIO_CMAPioctl 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-EFAULTwhen appropriate.Signed-off-by Michael Gehring <[email protected]>--- drivers/tty/vt/vt.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)drivers/tty/vt/vt.c +3
-3@@ -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++);
drivers/tty/vt/vt.c static int set_get_cmap(unsigned char __user *arg, int set) · line 3911
Quoted 10 lines
drivers/tty/vt/vt.c +6
-3+__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
Quoted 6 lines · Greg Kroah-Hartman
Quoted 2 lines · Michael Gehring
set_get_cmap()ignores the result of {get,put}_user(), causingioctl(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.
Quoted 5 lines
Quoted 5 lines
drivers/tty/vt/vt.c +0
-0@@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)
...
Quoted 8 lines
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):
drivers/tty/vt/vt.c +35 -6
@@ -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)
Quoted 21 lines · Michael Gehring
Quoted 6 lines · Greg Kroah-Hartman
Quoted 2 lines · Michael Gehring
set_get_cmap()ignores the result of {get,put}_user(), causingioctl(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.
Quoted 5 lines
Quoted 5 lines
drivers/tty/vt/vt.c +0
-0@@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)...
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
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).
Quoted 54 lines
drivers/tty/vt/vt.c +31
-5@@ -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]; + }
drivers/tty/vt/vt.c static int set_get_cmap(unsigned char __user *arg, int set) · line 3980
Quoted 10 lines
drivers/tty/vt/vt.c +4
-1console_unlock (); -return rc; +if (copy_to_user (arg, colormap,sizeof (colormap))) +return -EFAULT ; + +return 0 ; }void reset_palette (struct vc_data *vc)
thanks,
Signature · 2 lines
js suse labs
Quoted 2 lines
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
PATCH 1/1 tty/vt: handle bad user buffer in {G,P}IO_CMAP ioctl
In reply to Greg Kroah-Hartmanset_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.
--- drivers/tty/vt/vt.c | 68 ++++++++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 36 deletions(-)
drivers/tty/vt/vt.c +32 -36
@@ -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)
Signature · 1 line
1.7.9.4