On 8/30/2023 5:48 PM, Kees Cook wrote: > Warning: This email is from an unusual correspondent. > Warning: Make sure this is someone you trust. > > On Wed, Aug 30, 2023 at 05:17:12PM -0600, Dan Raymond wrote: >> 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; > > This would work if not for func_buf_lock. The bounce buffer is used to > avoid needing to hold the spin lock across copy_to_user. > Ah you're right. Thanks for setting me straight. Now I realize that my entire assessment was wrong. The original author was not using strlcpy() as a defensive measure to prevent a buffer overflow. He was using it so that he could create a copy of the string and measure its length using only one pass. This minimizes the time spent holding the spinlock. The surrounding code was written such that a buffer overflow is impossible. No additional checks are needed. The proposed patch is unnecessary. But at least it preserves the spirit of the original author's code by performing only one pass of the source string while holding the spinlock.
