* [PATCH] x86: simplefb: avoid overflow @ 2013-09-06 9:32 Tom Gundersen 2013-09-06 9:55 ` David Herrmann 0 siblings, 1 reply; 7+ messages in thread From: Tom Gundersen @ 2013-09-06 9:32 UTC (permalink / raw) To: x86 Cc: linux-kernel, mingo, tglx, Tom Gundersen, David Herrmann, H. Peter Anvin lfb_size can easily be say 4M, which would make the bitshit overflow and the test fail. Signed-off-by: Tom Gundersen <teg@jklm.no> Cc: David Herrmann <dh.herrmann@gmail.com> Cc: H. Peter Anvin <hpa@zytor.com> --- arch/x86/kernel/sysfb_simplefb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c index 22513e9..fff44a5 100644 --- a/arch/x86/kernel/sysfb_simplefb.c +++ b/arch/x86/kernel/sysfb_simplefb.c @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info *si, * the part that is occupied by the framebuffer */ len = mode->height * mode->stride; len = PAGE_ALIGN(len); - if (len > si->lfb_size << 16) { + if (len > ((unsigned long) si->lfb_size) << 16) { printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n"); return -EINVAL; } -- 1.8.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: simplefb: avoid overflow 2013-09-06 9:32 [PATCH] x86: simplefb: avoid overflow Tom Gundersen @ 2013-09-06 9:55 ` David Herrmann 2013-09-06 10:59 ` Geert Uytterhoeven 0 siblings, 1 reply; 7+ messages in thread From: David Herrmann @ 2013-09-06 9:55 UTC (permalink / raw) To: Tom Gundersen Cc: x86, linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin Hi On Fri, Sep 6, 2013 at 11:32 AM, Tom Gundersen <teg@jklm.no> wrote: > lfb_size can easily be say 4M, which would make the bitshit overflow and > the test fail. > > Signed-off-by: Tom Gundersen <teg@jklm.no> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: H. Peter Anvin <hpa@zytor.com> > --- > arch/x86/kernel/sysfb_simplefb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c > index 22513e9..fff44a5 100644 > --- a/arch/x86/kernel/sysfb_simplefb.c > +++ b/arch/x86/kernel/sysfb_simplefb.c > @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info *si, > * the part that is occupied by the framebuffer */ > len = mode->height * mode->stride; > len = PAGE_ALIGN(len); > - if (len > si->lfb_size << 16) { > + if (len > ((unsigned long) si->lfb_size) << 16) { Nice catch. vesafb uses "lfb_size * 65535" which causes an implicit cast. I thought <<16 looks nicer but that doesn't do any implicit cast.. Nothing crucial, as it only causes the simple-fb conversion to fail. But would still be good to see in -rc2: Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Thanks David > printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n"); > return -EINVAL; > } > -- > 1.8.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: simplefb: avoid overflow 2013-09-06 9:55 ` David Herrmann @ 2013-09-06 10:59 ` Geert Uytterhoeven 2013-09-06 11:08 ` H. Peter Anvin 2013-09-06 11:24 ` David Herrmann 0 siblings, 2 replies; 7+ messages in thread From: Geert Uytterhoeven @ 2013-09-06 10:59 UTC (permalink / raw) To: David Herrmann Cc: Tom Gundersen, the arch/x86 maintainers, linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin On Fri, Sep 6, 2013 at 11:55 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > On Fri, Sep 6, 2013 at 11:32 AM, Tom Gundersen <teg@jklm.no> wrote: >> lfb_size can easily be say 4M, which would make the bitshit overflow and >> the test fail. >> >> Signed-off-by: Tom Gundersen <teg@jklm.no> >> Cc: David Herrmann <dh.herrmann@gmail.com> >> Cc: H. Peter Anvin <hpa@zytor.com> >> --- >> arch/x86/kernel/sysfb_simplefb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c >> index 22513e9..fff44a5 100644 >> --- a/arch/x86/kernel/sysfb_simplefb.c >> +++ b/arch/x86/kernel/sysfb_simplefb.c >> @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info *si, >> * the part that is occupied by the framebuffer */ >> len = mode->height * mode->stride; >> len = PAGE_ALIGN(len); >> - if (len > si->lfb_size << 16) { >> + if (len > ((unsigned long) si->lfb_size) << 16) { On 32-bit, "unsigned long" is the same size as __u32, so this doesn't make any difference. > Nice catch. vesafb uses "lfb_size * 65535" which causes an implicit > cast. I thought <<16 looks nicer but that doesn't do any implicit > cast.. "lfb_size * 65535" is the same. "lfb_size" is __u32, "65535" is int. So there's no implicit cast. Or am I missing something? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: simplefb: avoid overflow 2013-09-06 10:59 ` Geert Uytterhoeven @ 2013-09-06 11:08 ` H. Peter Anvin 2013-09-06 11:15 ` Geert Uytterhoeven 2013-09-06 11:24 ` David Herrmann 1 sibling, 1 reply; 7+ messages in thread From: H. Peter Anvin @ 2013-09-06 11:08 UTC (permalink / raw) To: Geert Uytterhoeven Cc: David Herrmann, Tom Gundersen, the arch/x86 maintainers, linux-kernel, Ingo Molnar, Thomas Gleixner On 09/06/2013 03:59 AM, Geert Uytterhoeven wrote: > On Fri, Sep 6, 2013 at 11:55 AM, David Herrmann <dh.herrmann@gmail.com> wrote: >> On Fri, Sep 6, 2013 at 11:32 AM, Tom Gundersen <teg@jklm.no> wrote: >>> lfb_size can easily be say 4M, which would make the bitshit overflow and >>> the test fail. >>> >>> Signed-off-by: Tom Gundersen <teg@jklm.no> >>> Cc: David Herrmann <dh.herrmann@gmail.com> >>> Cc: H. Peter Anvin <hpa@zytor.com> >>> --- >>> arch/x86/kernel/sysfb_simplefb.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c >>> index 22513e9..fff44a5 100644 >>> --- a/arch/x86/kernel/sysfb_simplefb.c >>> +++ b/arch/x86/kernel/sysfb_simplefb.c >>> @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info *si, >>> * the part that is occupied by the framebuffer */ >>> len = mode->height * mode->stride; >>> len = PAGE_ALIGN(len); >>> - if (len > si->lfb_size << 16) { >>> + if (len > ((unsigned long) si->lfb_size) << 16) { > > On 32-bit, "unsigned long" is the same size as __u32, so this doesn't > make any difference. > >> Nice catch. vesafb uses "lfb_size * 65535" which causes an implicit >> cast. I thought <<16 looks nicer but that doesn't do any implicit >> cast.. > > "lfb_size * 65535" is the same. "lfb_size" is __u32, "65535" is int. > So there's no implicit cast. Or am I missing something? > << 16 is * 65536 not 65535... -hpa ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: simplefb: avoid overflow 2013-09-06 11:08 ` H. Peter Anvin @ 2013-09-06 11:15 ` Geert Uytterhoeven 0 siblings, 0 replies; 7+ messages in thread From: Geert Uytterhoeven @ 2013-09-06 11:15 UTC (permalink / raw) To: H. Peter Anvin Cc: David Herrmann, Tom Gundersen, the arch/x86 maintainers, linux-kernel, Ingo Molnar, Thomas Gleixner On Fri, Sep 6, 2013 at 1:08 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 09/06/2013 03:59 AM, Geert Uytterhoeven wrote: >> On Fri, Sep 6, 2013 at 11:55 AM, David Herrmann <dh.herrmann@gmail.com> >> wrote: >>> On Fri, Sep 6, 2013 at 11:32 AM, Tom Gundersen <teg@jklm.no> wrote: >>>> lfb_size can easily be say 4M, which would make the bitshit overflow and >>>> the test fail. >>>> >>>> Signed-off-by: Tom Gundersen <teg@jklm.no> >>>> Cc: David Herrmann <dh.herrmann@gmail.com> >>>> Cc: H. Peter Anvin <hpa@zytor.com> >>>> --- >>>> arch/x86/kernel/sysfb_simplefb.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kernel/sysfb_simplefb.c >>>> b/arch/x86/kernel/sysfb_simplefb.c >>>> index 22513e9..fff44a5 100644 >>>> --- a/arch/x86/kernel/sysfb_simplefb.c >>>> +++ b/arch/x86/kernel/sysfb_simplefb.c >>>> @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info >>>> *si, >>>> * the part that is occupied by the framebuffer */ >>>> len = mode->height * mode->stride; >>>> len = PAGE_ALIGN(len); >>>> - if (len > si->lfb_size << 16) { >>>> + if (len > ((unsigned long) si->lfb_size) << 16) { >> >> >> On 32-bit, "unsigned long" is the same size as __u32, so this doesn't >> make any difference. >> >>> Nice catch. vesafb uses "lfb_size * 65535" which causes an implicit >>> cast. I thought <<16 looks nicer but that doesn't do any implicit >>> cast.. >> >> >> "lfb_size * 65535" is the same. "lfb_size" is __u32, "65535" is int. >> So there's no implicit cast. Or am I missing something? >> > > << 16 is * 65536 not 65535... Oops, you're right. So that overflows a little bit later. Still, that doesn't change any reasoning w.r.t implicit casts and overflows. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: simplefb: avoid overflow 2013-09-06 10:59 ` Geert Uytterhoeven 2013-09-06 11:08 ` H. Peter Anvin @ 2013-09-06 11:24 ` David Herrmann 2013-09-06 11:57 ` Tom Gundersen 1 sibling, 1 reply; 7+ messages in thread From: David Herrmann @ 2013-09-06 11:24 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Tom Gundersen, the arch/x86 maintainers, linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin Hi On Fri, Sep 6, 2013 at 12:59 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Sep 6, 2013 at 11:55 AM, David Herrmann <dh.herrmann@gmail.com> wrote: >> On Fri, Sep 6, 2013 at 11:32 AM, Tom Gundersen <teg@jklm.no> wrote: >>> lfb_size can easily be say 4M, which would make the bitshit overflow and >>> the test fail. >>> >>> Signed-off-by: Tom Gundersen <teg@jklm.no> >>> Cc: David Herrmann <dh.herrmann@gmail.com> >>> Cc: H. Peter Anvin <hpa@zytor.com> >>> --- >>> arch/x86/kernel/sysfb_simplefb.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c >>> index 22513e9..fff44a5 100644 >>> --- a/arch/x86/kernel/sysfb_simplefb.c >>> +++ b/arch/x86/kernel/sysfb_simplefb.c >>> @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info *si, >>> * the part that is occupied by the framebuffer */ >>> len = mode->height * mode->stride; >>> len = PAGE_ALIGN(len); >>> - if (len > si->lfb_size << 16) { >>> + if (len > ((unsigned long) si->lfb_size) << 16) { > > On 32-bit, "unsigned long" is the same size as __u32, so this doesn't > make any difference. lfb_size cannot be 4M on 32bit machines. Well, if it is, the firmware passed bogus information as you cannot have a 262G-region on 32bit. But on 64bit it can pass 4M just fine. So we don't care for the 32bit case here, only 64bit. The (__u64) cast would be more obvious. Don't know.. It's just a sanity check, anyways, so I'm fine with it. >> Nice catch. vesafb uses "lfb_size * 65535" which causes an implicit >> cast. I thought <<16 looks nicer but that doesn't do any implicit >> cast.. > > "lfb_size * 65535" is the same. "lfb_size" is __u32, "65535" is int. > So there's no implicit cast. Or am I missing something? Yepp, indeed. My bad, so vesafb doesn't do any better here. I wonder, though, which firmware passes such values. It means the lfb_size area is reserved >4G. We do have huge VMEM these days.. Tom, on what hardware did you hit that? Thanks David ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] x86: simplefb: avoid overflow 2013-09-06 11:24 ` David Herrmann @ 2013-09-06 11:57 ` Tom Gundersen 0 siblings, 0 replies; 7+ messages in thread From: Tom Gundersen @ 2013-09-06 11:57 UTC (permalink / raw) To: David Herrmann Cc: Geert Uytterhoeven, the arch/x86 maintainers, linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin On Fri, Sep 6, 2013 at 1:24 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Fri, Sep 6, 2013 at 12:59 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Fri, Sep 6, 2013 at 11:55 AM, David Herrmann <dh.herrmann@gmail.com> wrote: >>> On Fri, Sep 6, 2013 at 11:32 AM, Tom Gundersen <teg@jklm.no> wrote: >>>> lfb_size can easily be say 4M, which would make the bitshit overflow and >>>> the test fail. >>>> >>>> Signed-off-by: Tom Gundersen <teg@jklm.no> >>>> Cc: David Herrmann <dh.herrmann@gmail.com> >>>> Cc: H. Peter Anvin <hpa@zytor.com> >>>> --- >>>> arch/x86/kernel/sysfb_simplefb.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c >>>> index 22513e9..fff44a5 100644 >>>> --- a/arch/x86/kernel/sysfb_simplefb.c >>>> +++ b/arch/x86/kernel/sysfb_simplefb.c >>>> @@ -72,7 +72,7 @@ __init int create_simplefb(const struct screen_info *si, >>>> * the part that is occupied by the framebuffer */ >>>> len = mode->height * mode->stride; >>>> len = PAGE_ALIGN(len); >>>> - if (len > si->lfb_size << 16) { >>>> + if (len > ((unsigned long) si->lfb_size) << 16) { >> >> On 32-bit, "unsigned long" is the same size as __u32, so this doesn't >> make any difference. > > lfb_size cannot be 4M on 32bit machines. Well, if it is, the firmware > passed bogus information as you cannot have a 262G-region on 32bit. > But on 64bit it can pass 4M just fine. > So we don't care for the 32bit case here, only 64bit. The (__u64) cast > would be more obvious. Don't know.. It's just a sanity check, anyways, > so I'm fine with it. I used unsigned long simply to match the type of len... >>> Nice catch. vesafb uses "lfb_size * 65535" which causes an implicit >>> cast. I thought <<16 looks nicer but that doesn't do any implicit >>> cast.. >> >> "lfb_size * 65535" is the same. "lfb_size" is __u32, "65535" is int. >> So there's no implicit cast. Or am I missing something? > > Yepp, indeed. My bad, so vesafb doesn't do any better here. > > I wonder, though, which firmware passes such values. It means the > lfb_size area is reserved >4G. We do have huge VMEM these days.. > Tom, on what hardware did you hit that? As always when weird things happen: DMI: Apple Inc. MacBookAir5,1/Mac-66F35F19FE2A0D05, BIOS MBA51.88Z.00EF.B01.1207271122 07/27/2012 Cheers, Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-06 11:57 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-06 9:32 [PATCH] x86: simplefb: avoid overflow Tom Gundersen 2013-09-06 9:55 ` David Herrmann 2013-09-06 10:59 ` Geert Uytterhoeven 2013-09-06 11:08 ` H. Peter Anvin 2013-09-06 11:15 ` Geert Uytterhoeven 2013-09-06 11:24 ` David Herrmann 2013-09-06 11:57 ` Tom Gundersen
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.