[PATCH] x86: simplefb: avoid overflow

[PATCH] x86: simplefb: avoid overflow

[Tom Gundersen]

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

Re: [PATCH] x86: simplefb: avoid overflow

[David Herrmann]

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
>

Re: [PATCH] x86: simplefb: avoid overflow

[Geert Uytterhoeven]

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

Re: [PATCH] x86: simplefb: avoid overflow

[H. Peter Anvin]

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

Re: [PATCH] x86: simplefb: avoid overflow

[Geert Uytterhoeven]

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

Re: [PATCH] x86: simplefb: avoid overflow

[David Herrmann]

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

Re: [PATCH] x86: simplefb: avoid overflow

[Tom Gundersen]

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