Re: [PATCH 11/12] hw/core/loader: read_targphys(): add upper bound
On Mon, 25 Sept 2023 at 20:41, 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
> 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
> ---
> 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
Re: [PATCH 11/12] hw/core/loader: read_targphys(): add upper bound
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
---
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!
--
Best regards,
Vladimir
Re: [PATCH 11/12] hw/core/loader: read_targphys(): add upper bound
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
---
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
