On Wednesday, Aug 11, 2021 at 6:53 PM, NeilBrown <neilb@suse.de (mailto:neilb@suse.de)> wrote:
On Thu, 12 Aug 2021, Nigel Croxon wrote:
quoted
To meet requirements of Common Criteria certification vulnerablility
assessment. Static code analysis has been run and found the following
errors:
check_return: Calling "fstat(fd, &dstb)" without checking return value.
This library function may fail and return an error code.
In what circumstances might it fail and return an error code?
NeilBrown
Hello Neil,
The fstat() function will fail if:
[EBADF] - The fildes argument is not a valid file descriptor.
But we never pass an invalid file descriptor
And you didn't list "EFAULT", but of course we never pass an invalid
memory address either.
[EIO] - An I/O error occurred while reading from the file system.
fstat() doesn't do IO, it just reports data from the cache.
[EOVERFLOW] - The file size in bytes or the number of blocks allocated to the file or the file serial number cannot be represented correctly in the structure pointed to by buf.
The fstat() function may fail if:
[EOVERFLOW] - One of the values is too large to store into the structure pointed to by the buf argument.
Those don't happen in practice for the fstat() calls that mdadm makes
either.
I think this patch is adding noise to the source code without actually
providing any real value. I would much prefer that if you really feel
there is value, then just add a wrapper:
int safe_fstat(....)
{
int ret = fstat(.....);
char message[]="mdadm: fstat failed, so aborting\n"
if (ret == 0)
return 0;
write(2, message, sizeof(message)-1);
exit(1);
}
Then just change every "fstat" in the code that bothers you to
"safe_fstat()".
This approach of adding pointless checks because some static analysis
tool thinks you should is not an approach that I approve of.
But, of course, it is up to Jes what patches he accepts...
NeilBrown
The fstat() function will fail if:
[EBADF] - The fildes argument is not a valid file descriptor.
But we never pass an invalid file descriptor
We can't guarantee that. There is always a minimal chance to pass
wrong/invalid argument caused by bug somewhere else in mdadm logic.
I think that handling such case is reasonable from security point
of view but agree that it could be a dead check (if everything is
well implemented).
And you didn't list "EFAULT", but of course we never pass an invalid
memory address either.
As before, we can't guarantee it, too. For now it seems to be well handled
but implementation may change and vulnerability might be missed during
review. Is safer to handle that some way.
quoted
[EIO] - An I/O error occurred while reading from the file system.
fstat() doesn't do IO, it just reports data from the cache.
quoted
[EOVERFLOW] - The file size in bytes or the number of blocks allocated to the file or the file serial number cannot be represented correctly in the structure pointed to by buf.
The fstat() function may fail if:
[EOVERFLOW] - One of the values is too large to store into the structure pointed to by the buf argument.
Those don't happen in practice for the fstat() calls that mdadm makes
either.
Agree, but it could be changed.
I think this patch is adding noise to the source code without actually
providing any real value. I would much prefer that if you really feel
there is value, then just add a wrapper:
int safe_fstat(....)
{
int ret = fstat(.....);
char message[]="mdadm: fstat failed, so aborting\n"
if (ret == 0)
return 0;
write(2, message, sizeof(message)-1);
exit(1);
}
Then just change every "fstat" in the code that bothers you to
"safe_fstat()".
This approach of adding pointless checks because some static analysis
tool thinks you should is not an approach that I approve of.
Maybe It won't be useful for users but it may help developers to avoid
trivial mistakes. As you told, if everything is fine then check is dead.
In my opinion any error handling is better than nothing.
Anyway, I think that there is a lot of more dangerous lines.
Regards,
Mariusz
Maybe It won't be useful for users but it may help developers to avoid
trivial mistakes. As you told, if everything is fine then check is dead.
In my opinion any error handling is better than nothing.
Error handling that is buggy, or that is hard to maintain is not better
than nothing. If I can't guarantee that we never pass a bad file
descriptor, then you cannot guarantee that the error handling has no
bugs. Less code generally means less bugs.
Any attempt to try to handle an error that should not be able to happen
other than crashing is fairly pointless - you cannot guess the real
cause, so you cannot know how to repair. Just printing a message and
continuing could be as bad as not checking the error.
NeilBrown
Error handling that is buggy, or that is hard to maintain is not better
than nothing. If I can't guarantee that we never pass a bad file
descriptor, then you cannot guarantee that the error handling has no
bugs. Less code generally means less bugs.
Any attempt to try to handle an error that should not be able to happen
other than crashing is fairly pointless - you cannot guess the real
cause, so you cannot know how to repair. Just printing a message and
continuing could be as bad as not checking the error.
As error handling, I meant any error verification. It doesn't indicate
that we should return status and end gracefully. exit() is elegant
solution in this case, totally agree.
Thanks,
Mariusz
Error handling that is buggy, or that is hard to maintain is not better
than nothing. If I can't guarantee that we never pass a bad file
descriptor, then you cannot guarantee that the error handling has no
bugs. Less code generally means less bugs.
Any attempt to try to handle an error that should not be able to happen
other than crashing is fairly pointless - you cannot guess the real
cause, so you cannot know how to repair. Just printing a message and
continuing could be as bad as not checking the error.
As error handling, I meant any error verification. It doesn't indicate
that we should return status and end gracefully. exit() is elegant
solution in this case, totally agree.
Just catching up here on this.
I totally agree that we need to work on catching errors and exiting
properly. It will also help returning error codes from this more silly
error handling cases to keep the certification people happy. This is a
much bigger job than just these checks though.
I don't think Nigel's patch is really harmful, but I don't think it adds
any real value either, without returning the actual error codes from
fstat and parsing them op the stack properly.
Jes