linux-kernel - Re: [PATCH v2] selftests/input: add test to cover len > maxlen in bits_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] [day] [month] [year] [list]

Date:   Thu, 22 Jun 2023 11:50:19 +0200
From:   Enric Balletbo Serra <eballetbo@...il.com>
To:     Dana Elfassy <delfassy@...hat.com>
Cc:     shuah@...nel.org, usama.anjum@...labora.com, eballetbo@...nel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        Dana Elfassy <dangel101@...il.com>
Subject: Re: [PATCH v2] selftests/input: add test to cover len > maxlen in bits_to_user()

Hi Dana,

Thank you for the patch.

Missatge de Dana Elfassy <delfassy@...hat.com> del dia dc., 21 de juny
2023 a les 10:30:
>
> In order to cover this case, setting 'maxlen = 0', with the following
> explanation:
> EVIOCGKEY is executed from evdev_do_ioctl(), which is called from
> evdev_ioctl_handler().
> evdev_ioctl_handler() is called from 2 functions, where by code coverage,
> only the first one is in use.
> ‘compat’ is given the value ‘0’ [1].
> Thus, the condition [2] is always false.
> This means ‘len’ always equals a positive number [3]
> ‘maxlen’ in evdev_handle_get_val [4] is defined locally in
> evdev_do_ioctl() [5], and is sent in the variable 'size' [6]
>

Like the comment in my previous patch I think this is understandable
for someone that has some context, but I am not sure it is understood
for people that don't have such context. So I'd try to rephrase and
explain in a more plain way. I.e.

selftests/input: introduce a test for the EVIOCGKEY ioctl

This patch introduces a specific test case for the EVIOCGKEY ioctl.
The test covers the case where len > maxlen in the
EVIOCGKEY(sizeof(keystate)), keystate) ioctl.


> [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1281
> [2] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L705
> [3] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L707
> [4] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L886
> [5] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1155
> [6] https://elixir.bootlin.com/linux/v6.2/source/drivers/input/evdev.c#L1141
>

Also, usually is not a good idea to add links in commit messages
reference to links that can easily end as dead links.

> Signed-off-by: Dana Elfassy <dangel101@...il.com>
> ---
> Changes in v2:
> - Added following note about the patch's dependency
>
> This patch depends on '[v3] selftests/input: Introduce basic tests for evdev ioctls' [1] sent to the ML.
> [1] https://patchwork.kernel.org/project/linux-input/patch/20230607153214.15933-1-eballetbo@kernel.org/
>
>  tools/testing/selftests/input/evioc-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/tools/testing/selftests/input/evioc-test.c b/tools/testing/selftests/input/evioc-test.c
> index ad7b93fe39cf..b94de2ee5596 100644
> --- a/tools/testing/selftests/input/evioc-test.c
> +++ b/tools/testing/selftests/input/evioc-test.c
> @@ -234,4 +234,23 @@ TEST(eviocsrep_set_repeat_settings)
>         selftest_uinput_destroy(uidev);
>  }
>
> +TEST(eviocgkey_get_global_key_state)
> +{
> +       struct selftest_uinput *uidev;
> +       int rep_values[2];

nit: rep_values sounds like repeat values, I'd use a variable name
more similar to what we really need to pass to the ioctl, like
keystate. Also I think this can be simply

int keystate;

> +       int rc;
> +
> +       memset(rep_values, 0, sizeof(rep_values));
> +

and then, this memset is not really needed.

> +       rc = selftest_uinput_create_device(&uidev);

This one without extra arguments need to be:

        rc = selftest_uinput_create_device(&uidev, -1);

otherwise the vararg loop is going to keep the room warm for no reason.

> +       ASSERT_EQ(0, rc);
> +       ASSERT_NE(NULL, uidev);
> +
> +       /* ioctl to create the scenario where len > maxlen in bits_to_user() */
> +       rc = ioctl(uidev->evdev_fd, EVIOCGKEY(0), rep_values);
> +       ASSERT_EQ(0, rc);
> +

while we are here, and as it should be easy, maybe we can also extend
this to cover the normal workflow to get the global key status?

> +       selftest_uinput_destroy(uidev);
> +}
> +
>  TEST_HARNESS_MAIN
> --
> 2.41.0
>

Thanks,
  Enric