On Tue, Oct 26, 2021 at 11:50 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Because for _most_ cases of "copy_to/from_user()" and friends by far,
> the only thing we look for is "zero for success".
Gaah. Looking around, I almost immediately found some really odd
exceptions to this.
Like parse_write_buffer_into_params() in amdgpu_dm_debugfs.c, which does
r = copy_from_user(wr_buf_ptr, buf, wr_buf_size);
/* r is bytes not be copied */
if (r >= wr_buf_size) {
DRM_DEBUG_DRIVER("user data not be read\n");
return -EINVAL;
}
and allows a partial copy to justy silently succeed, because all the
callers have pre-cleared the wr_buf_ptr buffer.
I have no idea why the code does that - it seems to imply that user
space could give an invalid 'size' parameter and mean to write only
the part that didn't succeed.
Adding AMD GPU driver people just to point out that this code not only
has odd whitespace, but that the pattern for "couldn't copy from user
space" should basically always be
if (copy_from_user(wr_buf_ptr, buf, wr_buf_size))
return -EFAULT;
because if user-space passes in a partially invalid buffer, you
generally really shouldn't say "ok, I got part of it and will use that
part"
There _are_ exceptions. We've had situations where user-space only
passes in the pointer to the buffer, but not the size. Bad interface,
but it historically happens for the 'mount()' system call 'data'
pointer. So then we'll copy "up to a page size".
Anyway, there are clearly some crazy users, and converting them all to
also check for negative error returns might be more painful than I
thought it would be.
Linus
