| Message ID | 20180609182235.33532-4-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix some coverity warnings | expand |
On 06/09/2018 08:22 PM, Simon Glass wrote: > The use of strcpy() to remove characters at the start of a string is safe > in U-Boot, since we know the implementation. But in os.c we are using the > C library's strcpy() function, where this behaviour is not permitted. > > Update the code to use memcpy() instead. > > Reported-by: Coverity (CID: 173279) > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > arch/sandbox/cpu/os.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c > index 5839932b00..496a8f9bd8 100644 > --- a/arch/sandbox/cpu/os.c > +++ b/arch/sandbox/cpu/os.c > @@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen) > /* Look for 'u-boot' in the parent directory of spl/ */ > p = strstr(fname, "/spl/"); I have built sandbox_spl_defconfig. First observation: any parallel build fails. But that is to be handled in a seperate patch. The logic to find u-boot here is broken. From the top directory of the git repository I executed: $ spl/u-boot-spl U-Boot SPL 2018.07-rc1-00063-g277daa5c0f (Jun 09 2018 - 20:40:23 +0200) Trying to boot from sandbox (spl/u-boot not found, error -2) SPL: failed to boot from all boot devices ### ERROR ### Please RESET the board ### Why would you expect fname to start with /spl/? In my case fname = 'spl/u-boot'. Why do you test for '/spl/' (only)? Just for the record if I execute $ ./spl/u-boot-spl everything works fine. But who would precede a directory name with './'? Should we not concatenate the spl executable and the u-boot executable during the build process so that spl/u-boot knows where to find u-boot? That would be much safer than any assumption about relative paths. Best regards Heinrich > if (p) { > - strcpy(p, p + 4); > + /* Remove the "/spl" characters */ > + memmove(p, p + 4, strlen(p + 4) + 1); > fd = os_open(fname, O_RDONLY); > if (fd >= 0) { > close(fd); >
Hi Heinrich, On 9 June 2018 at 10:56, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/09/2018 08:22 PM, Simon Glass wrote: >> The use of strcpy() to remove characters at the start of a string is safe >> in U-Boot, since we know the implementation. But in os.c we are using the >> C library's strcpy() function, where this behaviour is not permitted. >> >> Update the code to use memcpy() instead. >> >> Reported-by: Coverity (CID: 173279) >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> arch/sandbox/cpu/os.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c >> index 5839932b00..496a8f9bd8 100644 >> --- a/arch/sandbox/cpu/os.c >> +++ b/arch/sandbox/cpu/os.c >> @@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen) >> /* Look for 'u-boot' in the parent directory of spl/ */ >> p = strstr(fname, "/spl/"); > > I have built sandbox_spl_defconfig. > > First observation: any parallel build fails. But that is to be handled > in a seperate patch. I haven't seen that problem myself, but please send a patch or any details you have. > > The logic to find u-boot here is broken. From the top directory of the > git repository I executed: > > $ spl/u-boot-spl > > U-Boot SPL 2018.07-rc1-00063-g277daa5c0f (Jun 09 2018 - 20:40:23 +0200) > Trying to boot from sandbox > (spl/u-boot not found, error -2) > SPL: failed to boot from all boot devices > ### ERROR ### Please RESET the board ### > > Why would you expect fname to start with /spl/? > > In my case fname = 'spl/u-boot'. Why do you test for '/spl/' (only)? > > Just for the record if I execute > $ ./spl/u-boot-spl > everything works fine. But who would precede a directory name with './'? It should be save enough to drop the leading / from the strstr(), if that is what you are suggesting. > > Should we not concatenate the spl executable and the u-boot executable > during the build process so that spl/u-boot knows where to find u-boot? > That would be much safer than any assumption about relative paths. We could do that, and there is support for doing this at a low level (os_jump_to_image()). I've been working on some binman enhancements to build a Chrome OS image, which would use that feature. But executing from the filesystem seems reasonable to me, particularly for running tests. Regards, Simon > > Best regards > > Heinrich > >> if (p) { >> - strcpy(p, p + 4); >> + /* Remove the "/spl" characters */ >> + memmove(p, p + 4, strlen(p + 4) + 1); >> fd = os_open(fname, O_RDONLY); >> if (fd >= 0) { >> close(fd); >> >
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 5839932b00..496a8f9bd8 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -587,7 +587,8 @@ int os_find_u_boot(char *fname, int maxlen) /* Look for 'u-boot' in the parent directory of spl/ */ p = strstr(fname, "/spl/"); if (p) { - strcpy(p, p + 4); + /* Remove the "/spl" characters */ + memmove(p, p + 4, strlen(p + 4) + 1); fd = os_open(fname, O_RDONLY); if (fd >= 0) { close(fd);
The use of strcpy() to remove characters at the start of a string is safe in U-Boot, since we know the implementation. But in os.c we are using the C library's strcpy() function, where this behaviour is not permitted. Update the code to use memcpy() instead. Reported-by: Coverity (CID: 173279) Signed-off-by: Simon Glass <sjg@chromium.org> --- arch/sandbox/cpu/os.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)