[1/4] load_image_targphys() should enforce the max size

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

load_image_targphys() gets passed a max size for the file, but
doesn't enforce it at all. Add a check and return -1 (error) if
the file is too big, without loading it.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/loader.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Am 11.01.2012 06:44, schrieb David Gibson:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> load_image_targphys() gets passed a max size for the file, but
> doesn't enforce it at all. Add a check and return -1 (error) if
> the file is too big, without loading it.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/loader.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index 446b628..7ad9e22 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -108,6 +108,8 @@ int load_image_targphys(const char *filename,
> int size;
>
> size = get_image_size(filename);
> + if (size > max_sz)
> + return -1;
> if (size > 0)
> rom_add_file_fixed(filename, addr, -1);
> return size;

Even if this file is full of block statements without braces,
we should not add more of them. See CODING_STYLE and
scripts/checkpatch.pl.

There remains an additional problem:
Using 'int' for the size of files was sufficient 10 years ago,
but it is that no longer. get_image_size() silently reduced the
return value from lseek() to an 'int' value. So even with your
patch, very large files will be loaded (partially)!

Regards,
Stefan Weil

On Wed, Jan 11, 2012 at 07:19:00AM +0100, Stefan Weil wrote:
> Am 11.01.2012 06:44, schrieb David Gibson:
> >From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >
> >load_image_targphys() gets passed a max size for the file, but
> >doesn't enforce it at all. Add a check and return -1 (error) if
> >the file is too big, without loading it.
> >
> >Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >hw/loader.c | 2 ++
> >1 files changed, 2 insertions(+), 0 deletions(-)
> >
> >diff --git a/hw/loader.c b/hw/loader.c
> >index 446b628..7ad9e22 100644
> >--- a/hw/loader.c
> >+++ b/hw/loader.c
> >@@ -108,6 +108,8 @@ int load_image_targphys(const char *filename,
> >int size;
> >
> >size = get_image_size(filename);
> >+ if (size > max_sz)
> >+ return -1;
> >if (size > 0)
> >rom_add_file_fixed(filename, addr, -1);
> >return size;
> 
> Even if this file is full of block statements without braces,
> we should not add more of them. See CODING_STYLE and
> scripts/checkpatch.pl.

I know the coding style, I thought the counterexample right next to it
would take precedence.  Corrected in a respin.

> There remains an additional problem:
> Using 'int' for the size of files was sufficient 10 years ago,
> but it is that no longer. get_image_size() silently reduced the
> return value from lseek() to an 'int' value. So even with your
> patch, very large files will be loaded (partially)!

Well, sure, but that's an independent bug.  I'll fix it later if I get
to it, but that kind of leads to changing the types of a whole bunch
of things.