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: <aa488b1d-51b2-7b55-7a8d-552306ca16dd@foxvalley.net>
Date:   Wed, 30 Aug 2023 17:17:12 -0600
From:   Dan Raymond <draymond@...valley.net>
To:     Kees Cook <keescook@...omium.org>,
        Azeem Shaikh <azeemshaikh38@...il.com>
Cc:     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

>>>> The copy_to_user() call uses @len returned from strlcpy() directly
>>>> without checking its value. This could potentially lead to read
>>>> overflow.
>>>
>>> But can it?  How?
>>>
>>
>> The case I was considering is when the null-terminated hardcoded
>> string @func_table[kb_func] has length @new_len > @len. In this case,
>> strlcpy() will assign @len = @new_len and copy_to_user() would read
>> @new_len from the kmalloc-ed memory of @len. This is the potential
>> read overflow I was referring to. Let me know if I'm mistaken.
> 
> ..it's what I'd call "accidentally correct". i.e. it's using a fragile> anti-pattern, but in this case everything is hard-coded and less than
> 512.
> 
> Regardless, we need to swap for a sane pattern, which you've done. But
> the commit log is misleading, so it needs some more detail. :)
> 
> -Kees

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;