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

[Azeem Shaikh]

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?