| 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
| ||
|
Date: Fri, 17 May 2024 16:47:56 +0800
From: Joseph Jang <jjang@...dia.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: shuah@...nel.org, avagin@...gle.com, amir73il@...il.com,
brauner@...nel.org, mochs@...dia.com, jszu@...dia.com,
linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] selftest: rtc: Add to check rtc alarm status for alarm
related test
On 2024/5/17 4:08 PM, Alexandre Belloni wrote:
> On 17/05/2024 15:53:58+0800, Joseph Jang wrote:
>>
>>
>> On 2024/5/17 3:19 PM, Alexandre Belloni wrote:
>>> On 16/05/2024 19:28:47-0700, Joseph Jang wrote:
>>>> In alarm_wkalm_set and alarm_wkalm_set_minute test, they use different
>>>> ioctl (RTC_ALM_SET/RTC_WKALM_SET) for alarm feature detection. They will
>>>> skip testing if RTC_ALM_SET/RTC_WKALM_SET ioctl returns an EINVAL error
>>>> code. This design may miss detecting real problems when the
>>>> efi.set_wakeup_time() return errors and then RTC_ALM_SET/RTC_WKALM_SET
>>>> ioctl returns an EINVAL error code with RTC_FEATURE_ALARM enabled.
>>>>
>>>> In order to make rtctest more explicit and robust, we propose to use
>>>> RTC_PARAM_GET ioctl interface to check rtc alarm feature state before
>>>> running alarm related tests. If the kernel does not support RTC_PARAM_GET
>>>> ioctl interface, we will fallback to check the presence of "alarm" in
>>>> /proc/driver/rtc.
>>>>
>>>> The rtctest requires the read permission on /dev/rtc0. The rtctest will
>>>> be skipped if the /dev/rtc0 is not readable.
>>>>
>>>
>>> This change as to be separated. Also, I'm not sure what happened with
>>> https://lore.kernel.org/all/20230717175251.54390-1-atulpant.linux@gmail.com/
>>>
>>
>> I apply above patch and seems like still cannot detect the read
>> permission on /dev/rtc0. I guess the 'F_OK' just check the `/dev/rtc0`
>> was there.
>>
>> I share the error logs by following for your reference.
>>
>> TAP version 13
>> 1..1
>> # timeout set to 210
>> # selftests: rtc: rtctest
>> # TAP version 13
>> # 1..8
>> # # Starting 8 tests from 1 test cases.
>> # # RUN rtc.date_read ...
>> # # rtctest.c:53:date_read:Expected -1 (-1) != self->fd (-1)
>> # # date_read: Test terminated by assertion
>> # # FAIL rtc.date_read
>>
>> Not sure if we could skip the testing by following change ?
>>
>> FIXTURE_SETUP(rtc) {
>> + if (access(rtc_file, R_OK) != 0)
>> + SKIP(return, "Skipping test since cannot access %s, perhaps
>> miss sudo",
>> + rtc_file)
>> +
>> self->fd = open(rtc_file, O_RDONLY);
>> }
>>
>> And I make sure we need root permission to access `/dev/rtc0`.
>>
>
> There is no need to test for every tests of the suite, the check could
> be done once. To be clear, you don't need to be root to access the RTC,
> you need CAP_SYS_TIME and CAP_SYS_RESOURCE.
>
Thank you so much ~ not sure if we could have the following change ?
Use `access(rtc_file, R_OK)` to check read permission on
`/dev/rtc0` in main(). And user could try to figure out why cannot
access rtc file by themselves.
@@ -430,5 +433,11 @@ int main(int argc, char **argv)
return 1;
}
+ // Run the test if rtc_file is valid
+ if (access(rtc_file, R_OK) == 0)
+ ret = test_harness_run(argc, argv);
+ else
+ ksft_exit_fail_msg("[ERROR]: Cannot access rtc file %s - Exiting\n",
rtc_file);
+
+ return ret;
>>
>>
>>>> Requires commit 101ca8d05913b ("rtc: efi: Enable SET/GET WAKEUP services
>>>> as optional")
>>>>
>>>> Reviewed-by: Jeremy Szu <jszu@...dia.com>
>>>> Reviewed-by: Matthew R. Ochs <mochs@...dia.com>
>>>> Signed-off-by: Joseph Jang <jjang@...dia.com>
>>>> ---
>>>> tools/testing/selftests/rtc/Makefile | 2 +-
>>>> tools/testing/selftests/rtc/rtctest.c | 72 +++++++++++++++++++--------
>>>> 2 files changed, 53 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/rtc/Makefile b/tools/testing/selftests/rtc/Makefile
>>>> index 55198ecc04db..6e3a98fb24ba 100644
>>>> --- a/tools/testing/selftests/rtc/Makefile
>>>> +++ b/tools/testing/selftests/rtc/Makefile
>>>> @@ -1,5 +1,5 @@
>>>> # SPDX-License-Identifier: GPL-2.0
>>>> -CFLAGS += -O3 -Wl,-no-as-needed -Wall
>>>> +CFLAGS += -O3 -Wl,-no-as-needed -Wall -I../../../../usr/include/
>>>> LDLIBS += -lrt -lpthread -lm
>>>> TEST_GEN_PROGS = rtctest
>>>> diff --git a/tools/testing/selftests/rtc/rtctest.c b/tools/testing/selftests/rtc/rtctest.c
>>>> index 63ce02d1d5cc..aa47b17fbd1a 100644
>>>> --- a/tools/testing/selftests/rtc/rtctest.c
>>>> +++ b/tools/testing/selftests/rtc/rtctest.c
>>>> @@ -8,6 +8,7 @@
>>>> #include <errno.h>
>>>> #include <fcntl.h>
>>>> #include <linux/rtc.h>
>>>> +#include <stdbool.h>
>>>> #include <stdio.h>
>>>> #include <stdlib.h>
>>>> #include <sys/ioctl.h>
>>>> @@ -24,12 +25,17 @@
>>>> #define READ_LOOP_SLEEP_MS 11
>>>> static char *rtc_file = "/dev/rtc0";
>>>> +static char *rtc_procfs = "/proc/driver/rtc";
>>>> FIXTURE(rtc) {
>>>> int fd;
>>>> };
>>>> FIXTURE_SETUP(rtc) {
>>>> + if (access(rtc_file, R_OK) != 0)
>>>> + SKIP(return, "Skipping test since cannot access %s, perhaps miss sudo",
>>>> + rtc_file);
>>>
>>>> +
>>>> self->fd = open(rtc_file, O_RDONLY);
>>>> }
>>>> @@ -82,6 +88,36 @@ static void nanosleep_with_retries(long ns)
>>>> }
>>>> }
>>>> +static bool is_rtc_alarm_supported(int fd)
>>>> +{
>>>> + struct rtc_param param = { 0 };
>>>> + int rc;
>>>> + char buf[1024] = { 0 };
>>>> +
>>>> + /* Validate kernel reflects unsupported RTC alarm state */
>>>> + param.param = RTC_PARAM_FEATURES;
>>>> + param.index = 0;
>>>> + rc = ioctl(fd, RTC_PARAM_GET, ¶m);
>>>> + if (rc < 0) {
>>>> + /* Fallback to read rtc procfs */
>>>> + fd = open(rtc_procfs, O_RDONLY);
>>>
>>> I think I was clear on the previous thread, no new users of the procfs
>>> interface. You can carry this n your own tree but that can't be
>>> upstream.
>>>
>>
>> Okay ~ If we use RTC_PARAM_GET ioctl to detect rtc feature only, not
>> sure if that is okay for upstream ?
>
> Yes, using RTC_PARAM_GET is ok but I'm pretty sure this is not solving
> you are seeing following the efi patch you are pointing to.
>
> The patch clears RTC_FEATURE_ALARM when the alarm is not present which
> will ensure the ioctl fails and so the test will already be skipped. My
> guess is that your ssue ias actually when the alarm is present and the
> test will run and wait forever for an interrupt that will never come.
>
>
Thanks for helping to review our test again. Our original intention was
to check whether miss the efi kernel patch on target testing kernel.
I verified it and make sure we still could use RTC_PARAM_GET for our
testing on new kernel (> 5.16). If the target testing kernel miss the
efi patch, rtctest could detect the RTC_FEATURE_ALARM was enabled and
the RTC_WKALM_SET will report error due to efi.set_wakeup_time() return
EFI_UNSUPPORTED (3).
rc = ioctl(self->fd, RTC_WKALM_SET, &alarm);
ASSERT_NE(-1, rc);
And if the efi patch has been applied and it clears RTC_FEATURE_ALARM,
we could use the RTC_PARAM_GET to know rtc feature was disabled. So the
rtctest will skip the testing without reporting errors. That behavior is
what we expect.
Not sure if that work for you ?
Thank you,
Joseph.
Powered by blists - more mailing lists