linux-kernel - Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

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: <20150612074245.GC3890@piout.net>
Date:	Fri, 12 Jun 2015 09:42:45 +0200
From:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:	Andrea Scian <rnd4@...e-tech.it>
Cc:	r.cerrato@...-technologies.fr, a.zummo@...ertech.it,
	rtc-linux@...glegroups.com, linux-kernel@...r.kernel.org,
	Andrea Scian <andrea.scian@...e.eu>
Subject: Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect
 unreliable date and warn the user

On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote :
> >I would return -EINVAL here because the result might still pass
> >rtc_valid_tm() but be outdated.
> 
> At first look I agree with you, but a bit later they say:
> 
> /* the clock can give out invalid datetime, but we cannot return
>  * -EINVAL otherwise hwclock will refuse to set the time on bootup.
>  */
> 
> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91
> 
> so they don't actually return -EINVAL even if rtc_valid_tm() fails.
> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..
> 

This has been copy pasted from other drivers and this is simply not
true.

> If the comment above is correct, so we can't return -EINVAL, I will reset
> the time to epoch, with something like
> 
> rtc_time64_to_tm((time64_t)0, tm);
> 

Doing that is worse. You really want userspace to know that the time is
invalid instead of giving an incorrect value. This allow userspace to
actually choose its policy when the time is invalid. For example, use
epoch or any other later date that probabyl makes more sense for the
product.

> >>@@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
> >>  	switch (cmd) {
> >>  	case RTC_VL_READ:
> >>  		if (pcf2127->voltage_low)
> >>-			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
> >>+			dev_info(dev, "low voltage detected, check/replace battery\n");
> >
> >I would also print a warning about OFS here.
> >
> 
> I'll do.
> Do you think is better to add another variable inside struct pcf2127 or is
> better to re-read the RTC registers?
> (for the former I have also to clear the variable inside
> pcf2127_set_datetime(), for the latter I have to issue another read in a
> function that, at the moment, does not read anything..)
> 

I don't really care. But since one of them is already cached, it is
probably better to cache OFS. Maybe you could also use voltage_low as a
bit field which would allow userspace to make the difference between a
simple low voltage and the time loss condition.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/