| Message ID | 20200410175642.22525-1-toertel@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | core: Fix CID 292204 copyfile() out parameter check | expand |
Hi Mark, On 10/04/20 19:56, Mark Jonas wrote: > When the seek parameter is used, out must be pointing to a valid output > file descriptor. > > Signed-off-by: Mark Jonas <toertel@gmail.com> > --- > core/cpio_utils.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index cc96f7f..dc6d508 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -499,6 +499,12 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > > if (seek) { > int fdout = (out != NULL) ? *(int *)out : -1; > + if (fdout < 0) { > + ERROR("out argument: invalid fd or pointer"); > + ret = -EFAULT; > + goto copyfile_exit; > + } This seems convoluted - the macro in the first line sets intentionally the file descriptor to -1 to make seek() failing, what is considered bad in coverity. The lines above can be then simplified dropping the macro as: if (seek) { if (!out) { ERROR("out argument: invalid fd or pointer"); ret = -EINVAL; goto copyfile_exit; } fdout = *(int *)out; Best regards, Stefano > + > TRACE("offset has been defined: %llu bytes", seek); > if (lseek(fdout, seek, SEEK_SET) < 0) { > ERROR("offset argument: seek failed"); >
Hi Stefano, > > if (seek) { > > int fdout = (out != NULL) ? *(int *)out : -1; > > + if (fdout < 0) { > > + ERROR("out argument: invalid fd or pointer"); > > + ret = -EFAULT; > > + goto copyfile_exit; > > + } > > This seems convoluted - the macro in the first line sets intentionally > the file descriptor to -1 to make seek() failing, what is considered bad > in coverity. The lines above can be then simplified dropping the macro as: > > if (seek) { > if (!out) { > ERROR("out argument: invalid fd or pointer"); > ret = -EINVAL; > goto copyfile_exit; > } > fdout = *(int *)out; I kept the original convolution because I was not sure how much you love the ternary ?: operator. :) My code also catches the problem that 'out' might point to an invalid (negative) fd. I am pretty sure that Coverity will also detect this because '*(int *)out' can return positive and negative values. So the possible value range of 'fdout' includes negative numbers. And then the Coverity defect will persist. What about this: if (seek) { if (!out) { ERROR("out argument: pointer is NULL"); ret = -EINVAL; goto copyfile_exit; } fdout = *(int *)out; if (fdout < 0) { ERROR("out argument: points to invalid fd"); ret = -EINVAL; goto copyfile_exit; } Greetings, Mark
On 11/04/20 13:17, Mark Jonas wrote: > Hi Stefano, > >>> if (seek) { >>> int fdout = (out != NULL) ? *(int *)out : -1; >>> + if (fdout < 0) { >>> + ERROR("out argument: invalid fd or pointer"); >>> + ret = -EFAULT; >>> + goto copyfile_exit; >>> + } >> >> This seems convoluted - the macro in the first line sets intentionally >> the file descriptor to -1 to make seek() failing, what is considered bad >> in coverity. The lines above can be then simplified dropping the macro as: >> >> if (seek) { >> if (!out) { >> ERROR("out argument: invalid fd or pointer"); >> ret = -EINVAL; >> goto copyfile_exit; >> } >> fdout = *(int *)out; > > I kept the original convolution because I was not sure how much you > love the ternary ?: operator. :) It depends how I wake up... > > My code also catches the problem that 'out' might point to an invalid > (negative) fd. I am pretty sure that Coverity will also detect this > because '*(int *)out' can return positive and negative values. So the > possible value range of 'fdout' includes negative numbers. This should not happen because it is checked by any caller, that is they call fileoutput() and checks the return code. But sure coverity will complain gain. I will merge this to be coverity safe. Thanks, Stefano > And then > the Coverity defect will persist. > > What about this: > > if (seek) { > if (!out) { > ERROR("out argument: pointer is NULL"); > ret = -EINVAL; > goto copyfile_exit; > } > fdout = *(int *)out; > if (fdout < 0) { > ERROR("out argument: points to invalid fd"); > ret = -EINVAL; > goto copyfile_exit; > } > > Greetings, > Mark >
diff --git a/core/cpio_utils.c b/core/cpio_utils.c index cc96f7f..dc6d508 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -499,6 +499,12 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi if (seek) { int fdout = (out != NULL) ? *(int *)out : -1; + if (fdout < 0) { + ERROR("out argument: invalid fd or pointer"); + ret = -EFAULT; + goto copyfile_exit; + } + TRACE("offset has been defined: %llu bytes", seek); if (lseek(fdout, seek, SEEK_SET) < 0) { ERROR("offset argument: seek failed");
When the seek parameter is used, out must be pointing to a valid output file descriptor. Signed-off-by: Mark Jonas <toertel@gmail.com> --- core/cpio_utils.c | 6 ++++++ 1 file changed, 6 insertions(+)