linux-kernel - 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <82b80554-2042-7dcb-83c5-6a6b640c71be@foxvalley.net>
Date:   Wed, 30 Aug 2023 23:45:07 -0600
From:   Dan Raymond <draymond@...valley.net>
To:     Kees Cook <keescook@...omium.org>
Cc:     Azeem Shaikh <azeemshaikh38@...il.com>,
        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 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.