On Thu, Aug 31, 2023 at 1:45 AM Dan Raymond <draymond@xxxxxxxxxxxxx> wrote: > > 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. Are folks ok with me sending out a v2 for this with a better commit log that explains the issue?
