diff mbox series

[11/12] hw/core/loader: read_targphys(): add upper bound

Message ID 20230925194040.68592-12-vsementsov@yandex-team.ru
State New
Headers show
Series coverity fixes | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 25, 2023, 7:40 p.m. UTC
Coverity doesn't like using "untrusted" values, coming from buffers and
fd-s as length to do IO and allocations. And that's make sense. The
function is used three times with "untrusted" nbytes parameter. Let's
introduce at least empirical limit of 1G for it.

While being here make the function static, as it's used only here.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/core/loader.c    | 13 ++++++++++---
 include/hw/loader.h |  2 --
 2 files changed, 10 insertions(+), 5 deletions(-)

Comments

Michael Tokarev Sept. 25, 2023, 8:12 p.m. UTC | #1
25.09.2023 22:40, Vladimir Sementsov-Ogievskiy wrote:
> Coverity doesn't like using "untrusted" values, coming from buffers and
> fd-s as length to do IO and allocations. And that's make sense. The

"And that makes sense".  Just a nitpick in commit comment.

> function is used three times with "untrusted" nbytes parameter. Let's
> introduce at least empirical limit of 1G for it.
> 
> While being here make the function static, as it's used only here.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/core/loader.c    | 13 ++++++++++---
>   include/hw/loader.h |  2 --
>   2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa02b27089..48cff6f59e 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
>       return actsize < 0 ? -1 : l;
>   }
>   
> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
>   /* read()-like version */
> -ssize_t read_targphys(const char *name,
> -                      int fd, hwaddr dst_addr, size_t nbytes)
> +static ssize_t read_targphys(const char *name,
> +                             int fd, hwaddr dst_addr, size_t nbytes)
>   {
>       uint8_t *buf;
>       ssize_t did;
>   
> +    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
> +        return -1;

Right now this is not important, since the only user of this
function, load_aout(), ignores errno value and reports general
failure instead.  Original read_targphys() returned errno which
corresponds to failed read().

FWIW, at least load_aout() assumes we've read whole struct exec
from the file in question, which might not be the case.

/mjt
Vladimir Sementsov-Ogievskiy Sept. 26, 2023, 10:14 a.m. UTC | #2
On 25.09.23 23:12, Michael Tokarev wrote:
> 25.09.2023 22:40, Vladimir Sementsov-Ogievskiy wrote:
>> Coverity doesn't like using "untrusted" values, coming from buffers and
>> fd-s as length to do IO and allocations. And that's make sense. The
> 
> "And that makes sense".  Just a nitpick in commit comment.
> 
>> function is used three times with "untrusted" nbytes parameter. Let's
>> introduce at least empirical limit of 1G for it.
>>
>> While being here make the function static, as it's used only here.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   hw/core/loader.c    | 13 ++++++++++---
>>   include/hw/loader.h |  2 --
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index aa02b27089..48cff6f59e 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
>>       return actsize < 0 ? -1 : l;
>>   }
>> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
>>   /* read()-like version */
>> -ssize_t read_targphys(const char *name,
>> -                      int fd, hwaddr dst_addr, size_t nbytes)
>> +static ssize_t read_targphys(const char *name,
>> +                             int fd, hwaddr dst_addr, size_t nbytes)
>>   {
>>       uint8_t *buf;
>>       ssize_t did;
>> +    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
>> +        return -1;
> 
> Right now this is not important, since the only user of this
> function, load_aout(), ignores errno value and reports general
> failure instead.  Original read_targphys() returned errno which
> corresponds to failed read().

Agree, will fix to -EINVAL

> 
> FWIW, at least load_aout() assumes we've read whole struct exec
> from the file in question, which might not be the case.
> 

Hmm, right. Will fix too.

Thanks for reviewing!
Peter Maydell Sept. 26, 2023, 11:11 a.m. UTC | #3
On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> Coverity doesn't like using "untrusted" values, coming from buffers and
> fd-s as length to do IO and allocations. And that's make sense. The
> function is used three times with "untrusted" nbytes parameter. Let's
> introduce at least empirical limit of 1G for it.
>
> While being here make the function static, as it's used only here.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/core/loader.c    | 13 ++++++++++---
>  include/hw/loader.h |  2 --
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa02b27089..48cff6f59e 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void *addr, size_t size)
>      return actsize < 0 ? -1 : l;
>  }
>
> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
>  /* read()-like version */
> -ssize_t read_targphys(const char *name,
> -                      int fd, hwaddr dst_addr, size_t nbytes)
> +static ssize_t read_targphys(const char *name,
> +                             int fd, hwaddr dst_addr, size_t nbytes)
>  {
>      uint8_t *buf;
>      ssize_t did;
>
> +    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
> +        return -1;
> +    }
> +
>      buf = g_malloc(nbytes);
>      did = read(fd, buf, nbytes);
> -    if (did > 0)
> +    if (did > 0) {
>          rom_add_blob_fixed("read", buf, did, dst_addr);
> +    }
> +
>      g_free(buf);
>      return did;
>  }

This is called only from load_aout(). load_aout() is
passed a max size that it can write. So we shouldn't
be imposing a different maximum size in this utility function,
in the same way that the standard library read() does not
say "I'm going to impose a 1GB limit on how much you can read".
If the limit checks in load_aout() are wrong we should fix them.

(In any case load_aout() is used only for loading the initial
kernel on PPC and SPARC, so it's not very important.)

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/core/loader.c b/hw/core/loader.c
index aa02b27089..48cff6f59e 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -101,17 +101,24 @@  ssize_t load_image_size(const char *filename, void *addr, size_t size)
     return actsize < 0 ? -1 : l;
 }
 
+#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
 /* read()-like version */
-ssize_t read_targphys(const char *name,
-                      int fd, hwaddr dst_addr, size_t nbytes)
+static ssize_t read_targphys(const char *name,
+                             int fd, hwaddr dst_addr, size_t nbytes)
 {
     uint8_t *buf;
     ssize_t did;
 
+    if (nbytes > READ_TARGPHYS_MAX_BYTES) {
+        return -1;
+    }
+
     buf = g_malloc(nbytes);
     did = read(fd, buf, nbytes);
-    if (did > 0)
+    if (did > 0) {
         rom_add_blob_fixed("read", buf, did, dst_addr);
+    }
+
     g_free(buf);
     return did;
 }
diff --git a/include/hw/loader.h b/include/hw/loader.h
index c4c14170ea..e29af233d2 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -264,8 +264,6 @@  ssize_t load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
 
 ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src, size_t srclen);
 
-ssize_t read_targphys(const char *name,
-                      int fd, hwaddr dst_addr, size_t nbytes);
 void pstrcpy_targphys(const char *name,
                       hwaddr dest, int buf_size,
                       const char *source);