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]
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;

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ