| Message ID | 20230925194040.68592-12-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coverity fixes | expand |
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
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!
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 --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);
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(-)