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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Aug 2023 10:23:10 -0400
From:   Azeem Shaikh <azeemshaikh38@...il.com>
To:     Dan Raymond <draymond@...valley.net>
Cc:     Kees Cook <keescook@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] vt: Fix potential read overflow of kernel memory

On Thu, Aug 31, 2023 at 1:45 AM Dan Raymond <draymond@...valley.net> 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?