>>>> 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;
