Re: [PATCH] vt: Fix potential read overflow of kernel memory

[Dan Raymond]

>>>> The copy_to_user() call uses @len returned from strlcpy() directly
>>>> without checking its value. This could potentially lead to read
>>>> overflow.
>>>
>>> But can it?  How?
>>>
>>
>> The case I was considering is when the null-terminated hardcoded
>> string @func_table[kb_func] has length @new_len > @len. In this case,
>> strlcpy() will assign @len = @new_len and copy_to_user() would read
>> @new_len from the kmalloc-ed memory of @len. This is the potential
>> read overflow I was referring to. Let me know if I'm mistaken.
> 
> ..it's what I'd call "accidentally correct". i.e. it's using a fragile> anti-pattern, but in this case everything is hard-coded and less than
> 512.
> 
> Regardless, we need to swap for a sane pattern, which you've done. But
> the commit log is misleading, so it needs some more detail. :)
> 
> -Kees

In my opinion strlcpy() is being used correctly here as a defensive
precaution.  If the source string is larger than the destination buffer
it will truncate rather than corrupt kernel memory.  However the
return value of strlcpy() is being misused.  If truncation occurred
the copy_to_user() call will corrupt user memory instead.

I also agree that this is not currently a bug.  It is fragile and it
could break if someone added a very large string to the table.

Why not fix this by avoiding the redundant string copy?  How about
something like this:

ptr = func_table[kb_func] ? : "";
len = strlen(ptr);

if (len >= sizeof(user_kdgkb->kb_string))
	return -ENOSPC;

if (copy_to_user(user_kdgkb->kb_string, ptr, len + 1))
	return -EFAULT;