* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied [not found] <02E43B9E8855E74E886358B5184B4CC9104620C0@mail2-aub1fr.esi-supinfo.com> @ 2009-03-04 10:09 ` Jiri Kosina 2009-03-04 22:20 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Jiri Kosina @ 2009-03-04 10:09 UTC (permalink / raw) To: CUELLA Samuel, Andrew Morton Cc: adaplas@gmail.com, linux-kernel, trivial@kernel.org On Thu, 26 Feb 2009, CUELLA Samuel wrote: > from: Samuel CUELLA <samuel.cuella@supinfo.com> > > This patch prevents the kernel from being crashed by a divide-by-zero operation when supplied an incorrectly filled 'struct fb_var_screeninfo' from userland. > > Previously i810_main.c:1005 (i810_check_params) was using the global 'yres' symbol previously defined at i810_main.c:145 > as a module parameter value holder (i810_main.c:2174). If i810fb is compiled-in or if this param doesn't get a default value, > this direct usage leads to a divide-by-zero at i810_main.c:1005 (i810_check_params). The patch simply replace the 'yres' global, > perhaps undefined symbol usage by a given parameter structure lookup. > > This problem occurs with directfb, mplayer -vo fbdev, SDL library. > It was also reported ( but non solved ) at : http://mail.directfb.org/pipermail/directfb-dev/2008-March/004050.html > Sample code to reproduce : > /*Comile with gcc crashfb.c -o crashfb*/ > #include <fcntl.h> > #include <linux/fb.h> > #include <stdio.h> > #include <sys/ioctl.h> > #include <sys/mman.h> > #include <sys/stat.h> > #include <string.h> > #include <stdlib.h> > > > #define FB "/dev/fb0" > > int main(){ > int fd; > int rv; > struct fb_var_screeninfo vinfo; > > fd = open(FB,O_RDWR); > if( fd ){ > vinfo.xres = 800; > vinfo.yres = 600; > rv =ioctl(fd, FBIOPUT_VSCREENINFO, &vinfo); > } > return(rv); > } > Leads to this crash dump: > divide error: 0000 [#1] > last sysfs file: /sys/kernel/uevent_seqnum > Modules linked in: > > Pid: 4058, comm: crashfb Not tainted (2.6.28 #4) > EIP: 0060:[<c02558c8>] EFLAGS: 00010202 CPU: 0 > EIP is at i810fb_check_var+0x428/0x520 > EAX: 00400000 EBX: ce9d5e44 ECX: 001209a0 EDX: 00000000 > ESI: 00000020 EDI: 00000004 EBP: 00000000 ESP: ce9d5d0c > DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 > Process crashfb (pid: 4058, ti=ce9d4000 task=cf8af0e0 task.ti=ce9d4000) > Stack: > c014f993 00000000 00000001 00000000 00000000 00400000 0000001a cf811000 > 08048268 cf81123c 00000258 00000320 ffffffed cf811015 ce9d5e45 cf811000 > c0224821 ce9d5e44 ce9b09a0 00000000 00012000 00000000 c0111a89 00000001 > Call Trace: > [<c014f993>] handle_mm_fault+0x5c3/0x650 > [<c0224821>] fb_set_var+0x61/0x2d0 > [<c0111a89>] do_page_fault+0x3a9/0x8b0 > [<c016c935>] do_lookup+0x65/0x1a0 > [<c02257aa>] fb_ioctl+0x21a/0x3c0 > [<c014f577>] handle_mm_fault+0x1a7/0x650 > [<c0225590>] fb_ioctl+0x0/0x3c0 > [<c017077f>] vfs_ioctl+0x1f/0x70 > [<c017096c>] do_vfs_ioctl+0x5c/0x430 > [<c0111a89>] do_page_fault+0x3a9/0x8b0 > [<c0170d7d>] sys_ioctl+0x3d/0x70 > [<c0103af9>] sysenter_do_call+0x12/0x25 > Code: c0 0f 44 d0 89 54 24 04 e8 b6 5a ec ff b8 ea ff ff ff 83 c4 30 5b 5e 5f 5d c3 8b 2d ac 0e 4a c0 31 d2 89 f7 8b 44 24 14 c1 ef 03 <f7> f5 31 d2 f7 f7 3b 03 89 c7 0f 83 3c fd ff ff 89 c2 89 f1 89 > EIP: [<c02558c8>] i810fb_check_var+0x428/0x520 SS:ESP 0068:ce9d5d0c > ---[ end trace 1840767f449d222e ]--- > > Despite this dump says that EIP was in 'i810fb_check_var' the divide by zero truly occurs in 'i810_check_params' called by 'i810fb_check_var' (i810_main.c:1466). > > Signed-off-by: Samuel CUELLA <samuel.cuella@supinfo.com> > --- > --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100 > +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100 > @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v > struct i810fb_par *par = info->par; > int line_length, vidmem, mode_valid = 0, retval = 0; > u32 vyres = var->yres_virtual, vxres = var->xres_virtual; > + u32 yres = info->var.yres; > + > /* > * Memory limit > */ > This is not appropriate for trivial tree. CCing akpm and lkml. -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied 2009-03-04 10:09 ` [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied Jiri Kosina @ 2009-03-04 22:20 ` Andrew Morton 2009-03-04 22:54 ` Jiri Kosina 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2009-03-04 22:20 UTC (permalink / raw) To: Jiri Kosina Cc: Samuel.CUELLA, adaplas, linux-kernel, trivial, linux-fbdev-devel On Wed, 4 Mar 2009 11:09:21 +0100 (CET) Jiri Kosina <jkosina@suse.cz> wrote: > On Thu, 26 Feb 2009, CUELLA Samuel wrote: > > > from: Samuel CUELLA <samuel.cuella@supinfo.com> > > > > This patch prevents the kernel from being crashed by a divide-by-zero operation when supplied an incorrectly filled 'struct fb_var_screeninfo' from userland. > > > > Previously i810_main.c:1005 (i810_check_params) was using the global 'yres' symbol previously defined at i810_main.c:145 > > as a module parameter value holder (i810_main.c:2174). If i810fb is compiled-in or if this param doesn't get a default value, > > this direct usage leads to a divide-by-zero at i810_main.c:1005 (i810_check_params). The patch simply replace the 'yres' global, > > perhaps undefined symbol usage by a given parameter structure lookup. > > > > This problem occurs with directfb, mplayer -vo fbdev, SDL library. > > It was also reported ( but non solved ) at : http://mail.directfb.org/pipermail/directfb-dev/2008-March/004050.html > > Sample code to reproduce : > > /*Comile with gcc crashfb.c -o crashfb*/ > > #include <fcntl.h> > > #include <linux/fb.h> > > #include <stdio.h> > > #include <sys/ioctl.h> > > #include <sys/mman.h> > > #include <sys/stat.h> > > #include <string.h> > > #include <stdlib.h> > > > > > > #define FB "/dev/fb0" > > > > int main(){ > > int fd; > > int rv; > > struct fb_var_screeninfo vinfo; > > > > fd = open(FB,O_RDWR); > > if( fd ){ > > vinfo.xres = 800; > > vinfo.yres = 600; > > rv =ioctl(fd, FBIOPUT_VSCREENINFO, &vinfo); > > } > > return(rv); > > } > > Leads to this crash dump: > > divide error: 0000 [#1] > > last sysfs file: /sys/kernel/uevent_seqnum > > Modules linked in: > > > > Pid: 4058, comm: crashfb Not tainted (2.6.28 #4) > > EIP: 0060:[<c02558c8>] EFLAGS: 00010202 CPU: 0 > > EIP is at i810fb_check_var+0x428/0x520 > > EAX: 00400000 EBX: ce9d5e44 ECX: 001209a0 EDX: 00000000 > > ESI: 00000020 EDI: 00000004 EBP: 00000000 ESP: ce9d5d0c > > DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 > > Process crashfb (pid: 4058, ti=ce9d4000 task=cf8af0e0 task.ti=ce9d4000) > > Stack: > > c014f993 00000000 00000001 00000000 00000000 00400000 0000001a cf811000 > > 08048268 cf81123c 00000258 00000320 ffffffed cf811015 ce9d5e45 cf811000 > > c0224821 ce9d5e44 ce9b09a0 00000000 00012000 00000000 c0111a89 00000001 > > Call Trace: > > [<c014f993>] handle_mm_fault+0x5c3/0x650 > > [<c0224821>] fb_set_var+0x61/0x2d0 > > [<c0111a89>] do_page_fault+0x3a9/0x8b0 > > [<c016c935>] do_lookup+0x65/0x1a0 > > [<c02257aa>] fb_ioctl+0x21a/0x3c0 > > [<c014f577>] handle_mm_fault+0x1a7/0x650 > > [<c0225590>] fb_ioctl+0x0/0x3c0 > > [<c017077f>] vfs_ioctl+0x1f/0x70 > > [<c017096c>] do_vfs_ioctl+0x5c/0x430 > > [<c0111a89>] do_page_fault+0x3a9/0x8b0 > > [<c0170d7d>] sys_ioctl+0x3d/0x70 > > [<c0103af9>] sysenter_do_call+0x12/0x25 > > Code: c0 0f 44 d0 89 54 24 04 e8 b6 5a ec ff b8 ea ff ff ff 83 c4 30 5b 5e 5f 5d c3 8b 2d ac 0e 4a c0 31 d2 89 f7 8b 44 24 14 c1 ef 03 <f7> f5 31 d2 f7 f7 3b 03 89 c7 0f 83 3c fd ff ff 89 c2 89 f1 89 > > EIP: [<c02558c8>] i810fb_check_var+0x428/0x520 SS:ESP 0068:ce9d5d0c > > ---[ end trace 1840767f449d222e ]--- > > > > Despite this dump says that EIP was in 'i810fb_check_var' the divide by zero truly occurs in 'i810_check_params' called by 'i810fb_check_var' (i810_main.c:1466). > > > > Signed-off-by: Samuel CUELLA <samuel.cuella@supinfo.com> > > --- > > --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100 > > +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100 > > @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v > > struct i810fb_par *par = info->par; > > int line_length, vidmem, mode_valid = 0, retval = 0; > > u32 vyres = var->yres_virtual, vxres = var->xres_virtual; > > + u32 yres = info->var.yres; > > + > > /* > > * Memory limit > > */ > > > > This is not appropriate for trivial tree. CCing akpm and lkml. > I don't have a copy of the original patch. Please resend everything, with full changelog and a Signed-off-by: as per Documentation/SubmittingPatches. Please also cc linux-fbdev-devel@lists.sourceforge.net. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied 2009-03-04 22:20 ` Andrew Morton @ 2009-03-04 22:54 ` Jiri Kosina 2009-03-04 23:17 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Jiri Kosina @ 2009-03-04 22:54 UTC (permalink / raw) To: Andrew Morton Cc: Samuel.CUELLA, adaplas, linux-kernel, trivial, linux-fbdev-devel On Wed, 4 Mar 2009, Andrew Morton wrote: > > This is not appropriate for trivial tree. CCing akpm and lkml. > I don't have a copy of the original patch. This is what I got from Samuel (and rejected it for trivial tree, not being trivial enough :) ) From: Samuel CUELLA <samuel.cuella@supinfo.com> This patch prevents the kernel from being crashed by a divide-by-zero operation when supplied an incorrectly filled 'struct fb_var_screeninfo' from userland. Previously i810_main.c:1005 (i810_check_params) was using the global 'yres' symbol previously defined at i810_main.c:145 as a module parameter value holder (i810_main.c:2174). If i810fb is compiled-in or if this param doesn't get a default value, this direct usage leads to a divide-by-zero at i810_main.c:1005 (i810_check_params). The patch simply replace the 'yres' global, perhaps undefined symbol usage by a given parameter structure lookup. This problem occurs with directfb, mplayer -vo fbdev, SDL library. It was also reported ( but non solved ) at : http://mail.directfb.org/pipermail/directfb-dev/2008-March/004050.html Sample code to reproduce : /*Comile with gcc crashfb.c -o crashfb*/ #include <fcntl.h> #include <linux/fb.h> #include <stdio.h> #include <sys/ioctl.h> #include <sys/mman.h> #include <sys/stat.h> #include <string.h> #include <stdlib.h> #define FB "/dev/fb0" int main(){ int fd; int rv; struct fb_var_screeninfo vinfo; fd = open(FB,O_RDWR); if( fd ){ vinfo.xres = 800; vinfo.yres = 600; rv =ioctl(fd, FBIOPUT_VSCREENINFO, &vinfo); } return(rv); } Leads to this crash dump: divide error: 0000 [#1] last sysfs file: /sys/kernel/uevent_seqnum Modules linked in: Pid: 4058, comm: crashfb Not tainted (2.6.28 #4) EIP: 0060:[<c02558c8>] EFLAGS: 00010202 CPU: 0 EIP is at i810fb_check_var+0x428/0x520 EAX: 00400000 EBX: ce9d5e44 ECX: 001209a0 EDX: 00000000 ESI: 00000020 EDI: 00000004 EBP: 00000000 ESP: ce9d5d0c DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 Process crashfb (pid: 4058, ti=ce9d4000 task=cf8af0e0 task.ti=ce9d4000) Stack: c014f993 00000000 00000001 00000000 00000000 00400000 0000001a cf811000 08048268 cf81123c 00000258 00000320 ffffffed cf811015 ce9d5e45 cf811000 c0224821 ce9d5e44 ce9b09a0 00000000 00012000 00000000 c0111a89 00000001 Call Trace: [<c014f993>] handle_mm_fault+0x5c3/0x650 [<c0224821>] fb_set_var+0x61/0x2d0 [<c0111a89>] do_page_fault+0x3a9/0x8b0 [<c016c935>] do_lookup+0x65/0x1a0 [<c02257aa>] fb_ioctl+0x21a/0x3c0 [<c014f577>] handle_mm_fault+0x1a7/0x650 [<c0225590>] fb_ioctl+0x0/0x3c0 [<c017077f>] vfs_ioctl+0x1f/0x70 [<c017096c>] do_vfs_ioctl+0x5c/0x430 [<c0111a89>] do_page_fault+0x3a9/0x8b0 [<c0170d7d>] sys_ioctl+0x3d/0x70 [<c0103af9>] sysenter_do_call+0x12/0x25 Code: c0 0f 44 d0 89 54 24 04 e8 b6 5a ec ff b8 ea ff ff ff 83 c4 30 5b 5e 5f 5d c3 8b 2d ac 0e 4a c0 31 d2 89 f7 8b 44 24 14 c1 ef 03 <f7> f5 31 d2 f7 f7 3b 03 89 c7 0f 83 3c fd ff ff 89 c2 89 f1 89 EIP: [<c02558c8>] i810fb_check_var+0x428/0x520 SS:ESP 0068:ce9d5d0c ---[ end trace 1840767f449d222e ]--- Despite this dump says that EIP was in 'i810fb_check_var' the divide by zero truly occurs in 'i810_check_params' called by 'i810fb_check_var' (i810_main.c:1466). Signed-off-by: Samuel CUELLA <samuel.cuella@supinfo.com> --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100 +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100 @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v struct i810fb_par *par = info->par; int line_length, vidmem, mode_valid = 0, retval = 0; u32 vyres = var->yres_virtual, vxres = var->xres_virtual; + u32 yres = info->var.yres; + /* * Memory limit */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied 2009-03-04 22:54 ` Jiri Kosina @ 2009-03-04 23:17 ` Andrew Morton 2009-03-04 23:45 ` RE : " CUELLA Samuel 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2009-03-04 23:17 UTC (permalink / raw) To: Jiri Kosina Cc: Samuel.CUELLA, adaplas, linux-kernel, trivial, linux-fbdev-devel On Wed, 4 Mar 2009 23:54:49 +0100 (CET) Jiri Kosina <jkosina@suse.cz> wrote: > On Wed, 4 Mar 2009, Andrew Morton wrote: > > > > This is not appropriate for trivial tree. CCing akpm and lkml. > > I don't have a copy of the original patch. > > This is what I got from Samuel (and rejected it for trivial tree, not > being trivial enough :) ) > ok, thanks. > --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100 > +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100 > @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v > struct i810fb_par *par = info->par; > int line_length, vidmem, mode_valid = 0, retval = 0; > u32 vyres = var->yres_virtual, vxres = var->xres_virtual; > + u32 yres = info->var.yres; > + > /* > * Memory limit > */ So we have a file-wide static called `yres' - seems a bit dangerous. Rather than defining a local which shadows the global, it would be clearer to do it in this equivalent way, yes? --- a/drivers/video/i810/i810_main.c~i810-fix-kernel-crash-fix-when-struct-fb_var_screeninfo-is-supplied +++ a/drivers/video/i810/i810_main.c @@ -993,6 +993,7 @@ static int i810_check_params(struct fb_v struct i810fb_par *par = info->par; int line_length, vidmem, mode_valid = 0, retval = 0; u32 vyres = var->yres_virtual, vxres = var->xres_virtual; + /* * Memory limit */ @@ -1002,12 +1003,12 @@ static int i810_check_params(struct fb_v if (vidmem > par->fb.size) { vyres = par->fb.size/line_length; if (vyres < var->yres) { - vyres = yres; + vyres = info->var.yres; vxres = par->fb.size/vyres; vxres /= var->bits_per_pixel >> 3; line_length = get_line_length(par, vxres, var->bits_per_pixel); - vidmem = line_length * yres; + vidmem = line_length * info->var.yres; if (vxres < var->xres) { printk("i810fb: required video memory, " "%d bytes, for %dx%d-%d (virtual) " _ This assumes that the original change was actually correct. Should both sites whcih use the global `yres' have been converted to use info->var.yres, or just one of them? ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE : [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied 2009-03-04 23:17 ` Andrew Morton @ 2009-03-04 23:45 ` CUELLA Samuel 0 siblings, 0 replies; 5+ messages in thread From: CUELLA Samuel @ 2009-03-04 23:45 UTC (permalink / raw) To: Andrew Morton, Jiri Kosina Cc: adaplas@gmail.com, linux-kernel@vger.kernel.org, trivial@kernel.org, linux-fbdev-devel@lists.sourceforge.net On jeudi 5 mars 2009 00:17, Andrew Morton[akpm@linux-foundation.org] wrote : >On Wed, 4 Mar 2009 23:54:49 +0100 (CET) >Jiri Kosina <jkosina@suse.cz> wrote: >> On Wed, 4 Mar 2009, Andrew Morton wrote: >> >> > > This is not appropriate for trivial tree. CCing akpm and lkml. >> > I don't have a copy of the original patch. >> >> This is what I got from Samuel (and rejected it for trivial tree, not >> being trivial enough :) ) >> >ok, thanks. I was away, thanks for taking care of my patch :) >> --- linux-2.6.28/drivers/video/i810/i810_main.c.orig 2009-02-26 15:23:03.000000000 +0100 >> +++ linux-2.6.28/drivers/video/i810/i810_main.c 2009-02-26 14:50:06.000000000 +0100 >> @@ -993,6 +993,8 @@ static int i810_check_params(struct fb_v >> struct i810fb_par *par = info->par; >> int line_length, vidmem, mode_valid = 0, retval = 0; >> u32 vyres = var->yres_virtual, vxres = var->xres_virtual; >> + u32 yres = info->var.yres; >> + >> /* >> * Memory limit >> */ >So we have a file-wide static called `yres' - seems a bit dangerous. Yes, for sure, but there is the same kind of symbol for xres, so I leave it. As this variable is used to hold the 'yres' module parameter value, I think it's named upon this one. >Rather than defining a local which shadows the global, it would be >clearer to do it in this equivalent way, yes? Yes, you're totaly right. --- a/drivers/video/i810/i810_main.c~i810-fix-kernel-crash-fix-when-struct-fb_var_screeninfo-is-supplied +++ a/drivers/video/i810/i810_main.c @@ -993,6 +993,7 @@ static int i810_check_params(struct fb_v struct i810fb_par *par = info->par; int line_length, vidmem, mode_valid = 0, retval = 0; u32 vyres = var->yres_virtual, vxres = var->xres_virtual; + /* * Memory limit */ @@ -1002,12 +1003,12 @@ static int i810_check_params(struct fb_v if (vidmem > par->fb.size) { vyres = par->fb.size/line_length; if (vyres < var->yres) { - vyres = yres; + vyres = info->var.yres; vxres = par->fb.size/vyres; vxres /= var->bits_per_pixel >> 3; line_length = get_line_length(par, vxres, var->bits_per_pixel); - vidmem = line_length * yres; + vidmem = line_length * info->var.yres; if (vxres < var->xres) { printk("i810fb: required video memory, " "%d bytes, for %dx%d-%d (virtual) " _ >This assumes that the original change was actually correct. Should >both sites whcih use the global `yres' have been converted to use >info->var.yres, or just one of them? Yes, the two sites have to be changed. The orginal 'yres' global var is intended to be used as a module paramter value holder ( i810_main.c:2174), so it's not intended to be used to do other things than module initialization. P.S: Sorry for my bad english. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-03-04 23:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <02E43B9E8855E74E886358B5184B4CC9104620C0@mail2-aub1fr.esi-supinfo.com>
2009-03-04 10:09 ` [PATCH 2.6.28 ] i810: kernel crash fix when struct fb_var_screeninfo is supplied Jiri Kosina
2009-03-04 22:20 ` Andrew Morton
2009-03-04 22:54 ` Jiri Kosina
2009-03-04 23:17 ` Andrew Morton
2009-03-04 23:45 ` RE : " CUELLA Samuel
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.