linux-kernel - Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to

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: <CAHP4M8X5ZrrVBS6Y3tg6c8jK4BA0JK+q8reiGwVeoZX9gUvogw@mail.gmail.com>
Date:   Sat, 6 Nov 2021 17:35:49 +0530
From:   Ajay Garg <ajaygargnsit@...il.com>
To:     Pavel Skripkin <paskripkin@...il.com>
Cc:     linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

Hi Pavel,

Thanks for the review.

> >               len = strlcpy(kbs, func_table[kb_func] ? : "", len);
>
>                 ^^^^^^^^^
>
> len is reinitialized here, i.e len passed to kmalloc and len passed to
> copy_to_user() can be different.

Sorry, I missed this part.

>
> strlcpy() returns strlen() of source string (2nd argument), that's why
> we need +1 here to pass null byte to user.
>
> Am I missing something?
>
>

Seems things are more screwed.
I tried to see the behaviour, via a small program as below :

##########################
#include <stdio.h>
#include <bsd/string.h>

char a[10] = {0};
char b[] = "1234567890123456";

int main()
{
    int len = strlcpy(a, b, sizeof(a));
    printf("len = [%d]\n", len);
    printf("a = [%s]\n", a);

    return 0;
}
##########################

The result is :

##########################
len = [16]
a = [123456789]
##########################

As seen, len is *not equal* to the number of bytes actually copied.
(The bytes actually copied are 9 in number, plus 1 for the terminator,
as expected by strlcpy).

On re-reading the doc for strlcpy, it seems that strlcpy returns the
length of src it "intended* to copy, and not the bytes *actually
copied*. If so, then returned value of len is meaningless.

So, it seems following two changes should be made in the original code :

1.
                len = strlcpy(kbs, func_table[kb_func] ? : "", len);
=>
                strlcpy(kbs, func_table[kb_func] ? : "", len);

2.
ret = copy_to_user(user_kdgkb->kb_string, kbs, len) ?
                        -EFAULT : 0;
=>
ret = copy_to_user(user_kdgkb->kb_string, kbs, strlen(kbs) + 1) ?
                        -EFAULT : 0;

In 1, we change to simply not using the returned value of strlcpy.
In 2, we change to using strlen(kbs) + 1, as the number of bytes to copy.

Kindly know your thoughts.

Thanks and Regards,
Ajay