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

[Azeem Shaikh]

On Thu, Aug 31, 2023 at 1:32 AM Jiri Slaby <jirislaby@xxxxxxxxxx> wrote:
>
> On 30. 08. 23, 23:28, Kees Cook wrote:
> > On Wed, Aug 30, 2023 at 03:25:54PM -0400, Azeem Shaikh wrote:
> >> On Wed, Aug 30, 2023 at 1:57 PM Greg Kroah-Hartman
> >> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >>>
> >>> On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
> >>>> strlcpy() reads the entire source buffer first.
> >>>> This read may exceed the destination size limit if
> >>>> a source string is not NUL-terminated [1].
> >>>
> >>> But that's not the case here, right?  So your "potential read overflow"
> >>> isn't relevant here.
> >>>
> >>>> 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.
> >
> > First there is:
> >
> > ssize_t len = sizeof(user_kdgkb->kb_string);
> >
> > "struct user_kdgkb" is UAPI (therefore unlikely to change), and kb_string
> > is 512:
> >
> > struct kbsentry {
> >          unsigned char kb_func;
> >          unsigned char kb_string[512];
> > };
> >
> > Then we do:
> >
> >                  len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> >
> > This is the anti-pattern (take the length of the _source_) we need to
> > remove.
>
> But len is the length of kbs, i.e. the destination. Or what am I missing?
>
>                  kbs = kmalloc(len, GFP_KERNEL);
>                  len = strlcpy(kbs, func_table[kb_func] ? : "", len);
>
> > However, func_table[] is made up of either %NUL-terminated
> > strings:
> >
> > char func_buf[] = {
> >          '\033', '[', '[', 'A', 0,
> >          '\033', '[', '[', 'B', 0,
> > ...
> > char *func_table[MAX_NR_FUNC] = {
> >          func_buf + 0,
> >          func_buf + 5,
> > ...
> >
> > Or a NULL address itself. The ?: operator handles the NULL case, so
> > "len" can only ever be 0 through the longest string in func_buf. So 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. :)
>
> I am still missing what is wrong in the above code with strlcpy(). The
> dest is deliberately made as long as the source, so the returned len is
> never less then the passed len. No checking needed IMO. Perhaps, we
> might switch to strcpy()?
>
> FWIW I introduced this in commit 82e61c3909db5. So if it needs fixing,
> the patch deserves a Fixes: 82e61c3909db5 tag.
>

As explained by Kees in previous comments, this is not actually a bug,
just a fragile anti-pattern. So we shouldn't need the Fixes: tag on
this patch.

> thanks,
> --
> js
> suse labs
>