diff mbox

fix potential int overflow in efi/boot

Message ID alpine.DEB.2.10.1612081726420.22778@sstabellini-ThinkPad-X260 (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Stabellini Dec. 9, 2016, 1:30 a.m. UTC 
HorizontalResolution and VerticalResolution are 32bit, while size is
64bit. As it stands the multiplication is evaluated with 32bit
arithmetic, which could overflow. Cast HorizontalResolution to 64bit to
avoid that.

Coverity-ID: 1381858

Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>

Comments

Julien Grall Dec. 9, 2016, 2:27 p.m. UTC | #1 
Hi Stefano,

CC Jan as he is the maintainer of this code.

Cheers,

On 09/12/16 01:30, Stefano Stabellini wrote:
> HorizontalResolution and VerticalResolution are 32bit, while size is
> 64bit. As it stands the multiplication is evaluated with 32bit
> arithmetic, which could overflow. Cast HorizontalResolution to 64bit to
> avoid that.
>
> Coverity-ID: 1381858
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 56544dc..ff37bd9 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -687,7 +687,7 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>               mode_info->HorizontalResolution *
>               mode_info->VerticalResolution > size )
>          {
> -            size = mode_info->HorizontalResolution *
> +            size = (UINTN) mode_info->HorizontalResolution *
>                     mode_info->VerticalResolution;
>              gop_mode = i;
>          }
>
Jan Beulich Dec. 9, 2016, 2:43 p.m. UTC | #2 
>>> On 09.12.16 at 02:30, <sstabellini@kernel.org> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -687,7 +687,7 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>               mode_info->HorizontalResolution *
>               mode_info->VerticalResolution > size )
>          {
> -            size = mode_info->HorizontalResolution *
> +            size = (UINTN) mode_info->HorizontalResolution *
>                     mode_info->VerticalResolution;
>              gop_mode = i;
>          }

From a strictly formal perspective this is of course correct, but I'm
not convinced it matters in practice (imagine how huge a monitor we
need to overflow 4G pixels). But I'm willing to accept the patch,
provided you remove the stray space and (more importantly) you
also fix the other multiplication (visible in patch context) too.

Jan
diff mbox

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 56544dc..ff37bd9 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -687,7 +687,7 @@  static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
              mode_info->HorizontalResolution *
              mode_info->VerticalResolution > size )
         {
-            size = mode_info->HorizontalResolution *
+            size = (UINTN) mode_info->HorizontalResolution *
                    mode_info->VerticalResolution;
             gop_mode = i;
         }