linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rtc-cmos: Reject unsupported alarm values
@ 2016-08-31 22:59 Gabriele Mazzotta
  2016-08-31 23:41 ` Gabriele Mazzotta
  2016-09-02 19:55 ` [PATCH v2] " Gabriele Mazzotta
  0 siblings, 2 replies; 7+ messages in thread
From: Gabriele Mazzotta @ 2016-08-31 22:59 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni; +Cc: rtc-linux, linux-kernel, Gabriele Mazzotta

Return an error if the user tries to set an alarm that isn't
supported by the hardware.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/rtc/rtc-cmos.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 4cdb335..b3f9298 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -336,6 +336,26 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	if (!is_valid_irq(cmos->irq))
 		return -EIO;
 
+	if (!cmos->mon_alrm || !cmos->day_alrm) {
+		struct rtc_time now;
+		time64_t t_now;
+		time64_t t_alrm;
+
+		cmos_read_time(dev, &now);
+		t_now = rtc_tm_to_time64(&now);
+		t_alrm = rtc_tm_to_time64(&t->time);
+		if (!cmos->day_alrm && (t_alrm - t_now) > (24 * 60 * 60)) {
+			dev_err(dev,
+				"Alarms can be up to one day in the future\n");
+			return -EINVAL;
+		}
+		if (!cmos->mon_alrm && (t_alrm - t_now) > (31 * 24 * 60 * 60)) {
+			dev_err(dev,
+				"Alarms can be up to 31 days in the future\n");
+			return -EINVAL;
+		}
+	}
+
 	mon = t->time.tm_mon + 1;
 	mday = t->time.tm_mday;
 	hrs = t->time.tm_hour;
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] rtc-cmos: Reject unsupported alarm values
  2016-08-31 22:59 [PATCH] rtc-cmos: Reject unsupported alarm values Gabriele Mazzotta
@ 2016-08-31 23:41 ` Gabriele Mazzotta
  2016-09-02 19:55 ` [PATCH v2] " Gabriele Mazzotta
  1 sibling, 0 replies; 7+ messages in thread
From: Gabriele Mazzotta @ 2016-08-31 23:41 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni; +Cc: rtc-linux, linux-kernel

On 01/09/2016 00:59, Gabriele Mazzotta wrote:
> Return an error if the user tries to set an alarm that isn't
> supported by the hardware.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
>  drivers/rtc/rtc-cmos.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 4cdb335..b3f9298 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -336,6 +336,26 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>  	if (!is_valid_irq(cmos->irq))
>  		return -EIO;
>  
> +	if (!cmos->mon_alrm || !cmos->day_alrm) {
> +		struct rtc_time now;
> +		time64_t t_now;
> +		time64_t t_alrm;
> +
> +		cmos_read_time(dev, &now);
> +		t_now = rtc_tm_to_time64(&now);
> +		t_alrm = rtc_tm_to_time64(&t->time);
> +		if (!cmos->day_alrm && (t_alrm - t_now) > (24 * 60 * 60)) {
> +			dev_err(dev,
> +				"Alarms can be up to one day in the future\n");
> +			return -EINVAL;
> +		}
> +		if (!cmos->mon_alrm && (t_alrm - t_now) > (31 * 24 * 60 * 60)) {

I actually realized this is wrong. It's possible for this to let some
invalid dates go through. The driver writes a date in the registers,
so if mon_alrm is missing, I need to do something better than
adding 31 days. Sorry, I was thinking about time deltas rather
than well defined dates.

> +			dev_err(dev,
> +				"Alarms can be up to 31 days in the future\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	mon = t->time.tm_mon + 1;
>  	mday = t->time.tm_mday;
>  	hrs = t->time.tm_hour;
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] rtc-cmos: Reject unsupported alarm values
  2016-08-31 22:59 [PATCH] rtc-cmos: Reject unsupported alarm values Gabriele Mazzotta
  2016-08-31 23:41 ` Gabriele Mazzotta
@ 2016-09-02 19:55 ` Gabriele Mazzotta
  2016-09-21 23:29   ` Alexandre Belloni
  2016-10-03 22:50   ` [PATCH v3] " Gabriele Mazzotta
  1 sibling, 2 replies; 7+ messages in thread
From: Gabriele Mazzotta @ 2016-09-02 19:55 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni; +Cc: rtc-linux, linux-kernel, Gabriele Mazzotta

Some platforms allows to specify the month and day of the month in
which an alarm should go off, some others the day of the month and
some others just the time.

Currently any given value is accepted by the driver and only the
supported fields are used to program the hardware. As consequence,
alarms are potentially programmed to go off in the wrong moment.

Fix this by rejecting any value not supported by the hardware.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---

I revisited the naive implementation of v1. I tested the new
algorithm using some dates that and verified that it behaved as
expected, but I might have missed some corner cases.

I made some assumptions that maybe should be dropped, at least
two of them. They are commented in the code, but I didn't mention
that they are assumptions:

 - If the day can't be specified, the alarm can only be set to go
   off 24 hours minus 1 second in the future. I'm worried things
   would go wrong if the current time is used to set an alarm that
   should go off the next day.
 - If the mday can be specified and the next month has more days
   than the current month, the alarm can be set to go off in the
   extra days of the next month.
 - If the month can be specified, it's the 28th of February and the
   next year is a leap year, the alarm can be set for the 29th of
   February of the next year.

Basically I'm assuming that the hardware decides when an alarm should
go off comparing the current date with the one programmed. If they
match, the alarm goes off. This seemed reasonable to me, but it's
not easy to verify.

 drivers/rtc/rtc-cmos.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 4cdb335..37cb7c1 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -328,14 +328,118 @@ static void cmos_irq_disable(struct cmos_rtc *cmos, unsigned char mask)
 	cmos_checkintr(cmos, rtc_control);
 }
 
+static int cmos_validate_alarm(struct device *dev, struct rtc_wkalrm *t)
+{
+	struct cmos_rtc *cmos = dev_get_drvdata(dev);
+	struct rtc_time now;
+
+	cmos_read_time(dev, &now);
+
+	if (!cmos->day_alrm) {
+		time64_t t_max_date;
+		time64_t t_alrm;
+
+		t_alrm = rtc_tm_to_time64(&t->time);
+		t_max_date = rtc_tm_to_time64(&now);
+		/*
+		 * Subtract 1 second to ensure that the alarm time is
+		 * different from the current time.
+		 */
+		t_max_date += 24 * 60 * 60 - 1;
+		if (t_alrm > t_max_date) {
+			dev_err(dev,
+				"Alarms can be up to one day in the future\n");
+			return -EINVAL;
+		}
+	} else if (!cmos->mon_alrm) {
+		struct rtc_time max_date = now;
+		time64_t t_max_date;
+		time64_t t_alrm;
+		int max_mday;
+		bool is_max_mday = false;
+
+		/*
+		 * If the next month has more days than the current month
+		 * and we are at the max mday of this month, we can program
+		 * the alarm to go off the max mday of the next month without
+		 * it going off sooner than expected.
+		 */
+		max_mday = rtc_month_days(now.tm_mon, now.tm_year);
+		if (now.tm_mday == max_mday)
+			is_max_mday = true;
+
+		if (max_date.tm_mon == 11) {
+			max_date.tm_mon = 0;
+			max_date.tm_year += 1;
+		} else {
+			max_date.tm_mon += 1;
+		}
+		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
+		if (max_date.tm_mday > max_mday || is_max_mday)
+			max_date.tm_mday = max_mday;
+
+		max_date.tm_hour = 23;
+		max_date.tm_min = 59;
+		max_date.tm_sec = 59;
+
+		t_max_date = rtc_tm_to_time64(&max_date);
+		t_alrm = rtc_tm_to_time64(&t->time);
+		if (t_alrm > t_max_date) {
+			dev_err(dev,
+				"Alarms can be up to one month in the future\n");
+			return -EINVAL;
+		}
+	} else {
+		struct rtc_time max_date = now;
+		time64_t t_max_date;
+		time64_t t_alrm;
+		int max_mday;
+		bool allow_leap_day = false;
+
+		/*
+		 * If it's the 28th of February and the next year is a leap
+		 * year, allow to set alarms for the 29th of February.
+		 */
+		if (now.tm_mon == 1) {
+			max_mday = rtc_month_days(now.tm_mon, now.tm_year);
+			if (now.tm_mday == max_mday)
+				allow_leap_day = true;
+		}
+
+		max_date.tm_year += 1;
+		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
+		if (max_date.tm_mday > max_mday || allow_leap_day)
+			max_date.tm_mday = max_mday;
+
+		max_date.tm_hour = 23;
+		max_date.tm_min = 59;
+		max_date.tm_sec = 59;
+
+		t_max_date = rtc_tm_to_time64(&max_date);
+		t_alrm = rtc_tm_to_time64(&t->time);
+		if (t_alrm > t_max_date) {
+			dev_err(dev,
+				"Alarms can be up to one year in the future\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 	unsigned char mon, mday, hrs, min, sec, rtc_control;
+	int ret;
 
 	if (!is_valid_irq(cmos->irq))
 		return -EIO;
 
+	ret = cmos_validate_alarm(dev, t);
+	if (ret < 0)
+		return ret;
+
 	mon = t->time.tm_mon + 1;
 	mday = t->time.tm_mday;
 	hrs = t->time.tm_hour;
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] rtc-cmos: Reject unsupported alarm values
  2016-09-02 19:55 ` [PATCH v2] " Gabriele Mazzotta
@ 2016-09-21 23:29   ` Alexandre Belloni
  2016-09-22 19:47     ` Gabriele Mazzotta
  2016-10-03 22:50   ` [PATCH v3] " Gabriele Mazzotta
  1 sibling, 1 reply; 7+ messages in thread
From: Alexandre Belloni @ 2016-09-21 23:29 UTC (permalink / raw)
  To: Gabriele Mazzotta; +Cc: a.zummo, rtc-linux, linux-kernel

On 02/09/2016 at 21:55:16 +0200, Gabriele Mazzotta wrote :
> Some platforms allows to specify the month and day of the month in
> which an alarm should go off, some others the day of the month and
> some others just the time.
> 
> Currently any given value is accepted by the driver and only the
> supported fields are used to program the hardware. As consequence,
> alarms are potentially programmed to go off in the wrong moment.
> 
> Fix this by rejecting any value not supported by the hardware.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
> 
> I revisited the naive implementation of v1. I tested the new
> algorithm using some dates that and verified that it behaved as
> expected, but I might have missed some corner cases.
> 
> I made some assumptions that maybe should be dropped, at least
> two of them. They are commented in the code, but I didn't mention
> that they are assumptions:
> 
>  - If the day can't be specified, the alarm can only be set to go
>    off 24 hours minus 1 second in the future. I'm worried things
>    would go wrong if the current time is used to set an alarm that
>    should go off the next day.
>  - If the mday can be specified and the next month has more days
>    than the current month, the alarm can be set to go off in the
>    extra days of the next month.

Hum, you are actually not allowing them in the code below (which I think
is the right thing to do).

>  - If the month can be specified, it's the 28th of February and the
>    next year is a leap year, the alarm can be set for the 29th of
>    February of the next year.

Is that really true? I would expect the opposite. If it is currently
28/02, the max date you can actually go to is 27/02. If you allow 29/02,
at some time the RTC will have 28/02 and the alarm will fire.

> 
> Basically I'm assuming that the hardware decides when an alarm should
> go off comparing the current date with the one programmed. If they
> match, the alarm goes off. This seemed reasonable to me, but it's
> not easy to verify.
> 
>  drivers/rtc/rtc-cmos.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 4cdb335..37cb7c1 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -328,14 +328,118 @@ static void cmos_irq_disable(struct cmos_rtc *cmos, unsigned char mask)
>  	cmos_checkintr(cmos, rtc_control);
>  }
>  
> +static int cmos_validate_alarm(struct device *dev, struct rtc_wkalrm *t)
> +{
> +	struct cmos_rtc *cmos = dev_get_drvdata(dev);
> +	struct rtc_time now;
> +
> +	cmos_read_time(dev, &now);
> +
> +	if (!cmos->day_alrm) {
> +		time64_t t_max_date;
> +		time64_t t_alrm;
> +
> +		t_alrm = rtc_tm_to_time64(&t->time);
> +		t_max_date = rtc_tm_to_time64(&now);
> +		/*
> +		 * Subtract 1 second to ensure that the alarm time is
> +		 * different from the current time.
> +		 */
> +		t_max_date += 24 * 60 * 60 - 1;
> +		if (t_alrm > t_max_date) {
> +			dev_err(dev,
> +				"Alarms can be up to one day in the future\n");
> +			return -EINVAL;
> +		}
> +	} else if (!cmos->mon_alrm) {
> +		struct rtc_time max_date = now;
> +		time64_t t_max_date;
> +		time64_t t_alrm;
> +		int max_mday;
> +		bool is_max_mday = false;
> +
> +		/*
> +		 * If the next month has more days than the current month
> +		 * and we are at the max mday of this month, we can program
> +		 * the alarm to go off the max mday of the next month without
> +		 * it going off sooner than expected.
> +		 */
> +		max_mday = rtc_month_days(now.tm_mon, now.tm_year);
> +		if (now.tm_mday == max_mday)
> +			is_max_mday = true;
> +
> +		if (max_date.tm_mon == 11) {
> +			max_date.tm_mon = 0;
> +			max_date.tm_year += 1;
> +		} else {
> +			max_date.tm_mon += 1;
> +		}
> +		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
> +		if (max_date.tm_mday > max_mday || is_max_mday)
> +			max_date.tm_mday = max_mday;
> +
> +		max_date.tm_hour = 23;
> +		max_date.tm_min = 59;
> +		max_date.tm_sec = 59;
> +

Actually, this is wrong.

If it is currently 1:23:45 on 22/09, you can go up to 1:23:44 on 22/10.
trying to set a time after 1:23:45 will actually match on the same day
instead of a month later.

> +		t_max_date = rtc_tm_to_time64(&max_date);
> +		t_alrm = rtc_tm_to_time64(&t->time);
> +		if (t_alrm > t_max_date) {
> +			dev_err(dev,
> +				"Alarms can be up to one month in the future\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		struct rtc_time max_date = now;
> +		time64_t t_max_date;
> +		time64_t t_alrm;
> +		int max_mday;
> +		bool allow_leap_day = false;
> +
> +		/*
> +		 * If it's the 28th of February and the next year is a leap
> +		 * year, allow to set alarms for the 29th of February.
> +		 */
> +		if (now.tm_mon == 1) {
> +			max_mday = rtc_month_days(now.tm_mon, now.tm_year);
> +			if (now.tm_mday == max_mday)
> +				allow_leap_day = true;
> +		}
> +
> +		max_date.tm_year += 1;
> +		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
> +		if (max_date.tm_mday > max_mday || allow_leap_day)
> +			max_date.tm_mday = max_mday;
> +
> +		max_date.tm_hour = 23;
> +		max_date.tm_min = 59;
> +		max_date.tm_sec = 59;
> +

Ditto, 1:23:45 on 22/09/2016 can go up to 1:23:44 on 22/09/2017.

Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] rtc-cmos: Reject unsupported alarm values
  2016-09-21 23:29   ` Alexandre Belloni
@ 2016-09-22 19:47     ` Gabriele Mazzotta
  0 siblings, 0 replies; 7+ messages in thread
From: Gabriele Mazzotta @ 2016-09-22 19:47 UTC (permalink / raw)
  To: Alexandre Belloni; +Cc: Alessandro Zummo, rtc-linux, linux-kernel

2016-09-22 1:29 GMT+02:00 Alexandre Belloni
<alexandre.belloni@free-electrons.com>:
> On 02/09/2016 at 21:55:16 +0200, Gabriele Mazzotta wrote :
>> Some platforms allows to specify the month and day of the month in
>> which an alarm should go off, some others the day of the month and
>> some others just the time.
>>
>> Currently any given value is accepted by the driver and only the
>> supported fields are used to program the hardware. As consequence,
>> alarms are potentially programmed to go off in the wrong moment.
>>
>> Fix this by rejecting any value not supported by the hardware.
>>
>> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
>> ---
>>
>> I revisited the naive implementation of v1. I tested the new
>> algorithm using some dates that and verified that it behaved as
>> expected, but I might have missed some corner cases.
>>
>> I made some assumptions that maybe should be dropped, at least
>> two of them. They are commented in the code, but I didn't mention
>> that they are assumptions:
>>
>>  - If the day can't be specified, the alarm can only be set to go
>>    off 24 hours minus 1 second in the future. I'm worried things
>>    would go wrong if the current time is used to set an alarm that
>>    should go off the next day.
>>  - If the mday can be specified and the next month has more days
>>    than the current month, the alarm can be set to go off in the
>>    extra days of the next month.
>
> Hum, you are actually not allowing them in the code below (which I think
> is the right thing to do).

I do, unfortunately. If it's "2015/02/28 01:23:45", then
"2015/03/31 01:23:44" is considered valid and yes, this is wrong.

>>  - If the month can be specified, it's the 28th of February and the
>>    next year is a leap year, the alarm can be set for the 29th of
>>    February of the next year.
>
> Is that really true? I would expect the opposite. If it is currently
> 28/02, the max date you can actually go to is 27/02. If you allow 29/02,
> at some time the RTC will have 28/02 and the alarm will fire.

What I thought here is that since we write MM/DD-hh:mm:ss, if it's
02/28-01:34:56 (A) and I set the alarm for 02/29-01:34:55 (B),
that alarm can only go off next year. What I realize now is
that allowing (B), for example, makes 02/28-01:34:57 ambiguous
as it appears twice between (A) and (B). So yes, this is wrong.

>>
>> Basically I'm assuming that the hardware decides when an alarm should
>> go off comparing the current date with the one programmed. If they
>> match, the alarm goes off. This seemed reasonable to me, but it's
>> not easy to verify.
>>
>>  drivers/rtc/rtc-cmos.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 104 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
>> index 4cdb335..37cb7c1 100644
>> --- a/drivers/rtc/rtc-cmos.c
>> +++ b/drivers/rtc/rtc-cmos.c
>> @@ -328,14 +328,118 @@ static void cmos_irq_disable(struct cmos_rtc *cmos, unsigned char mask)
>>       cmos_checkintr(cmos, rtc_control);
>>  }
>>
>> +static int cmos_validate_alarm(struct device *dev, struct rtc_wkalrm *t)
>> +{
>> +     struct cmos_rtc *cmos = dev_get_drvdata(dev);
>> +     struct rtc_time now;
>> +
>> +     cmos_read_time(dev, &now);
>> +
>> +     if (!cmos->day_alrm) {
>> +             time64_t t_max_date;
>> +             time64_t t_alrm;
>> +
>> +             t_alrm = rtc_tm_to_time64(&t->time);
>> +             t_max_date = rtc_tm_to_time64(&now);
>> +             /*
>> +              * Subtract 1 second to ensure that the alarm time is
>> +              * different from the current time.
>> +              */
>> +             t_max_date += 24 * 60 * 60 - 1;
>> +             if (t_alrm > t_max_date) {
>> +                     dev_err(dev,
>> +                             "Alarms can be up to one day in the future\n");
>> +                     return -EINVAL;
>> +             }
>> +     } else if (!cmos->mon_alrm) {
>> +             struct rtc_time max_date = now;
>> +             time64_t t_max_date;
>> +             time64_t t_alrm;
>> +             int max_mday;
>> +             bool is_max_mday = false;
>> +
>> +             /*
>> +              * If the next month has more days than the current month
>> +              * and we are at the max mday of this month, we can program
>> +              * the alarm to go off the max mday of the next month without
>> +              * it going off sooner than expected.
>> +              */
>> +             max_mday = rtc_month_days(now.tm_mon, now.tm_year);
>> +             if (now.tm_mday == max_mday)
>> +                     is_max_mday = true;
>> +
>> +             if (max_date.tm_mon == 11) {
>> +                     max_date.tm_mon = 0;
>> +                     max_date.tm_year += 1;
>> +             } else {
>> +                     max_date.tm_mon += 1;
>> +             }
>> +             max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
>> +             if (max_date.tm_mday > max_mday || is_max_mday)
>> +                     max_date.tm_mday = max_mday;
>> +
>> +             max_date.tm_hour = 23;
>> +             max_date.tm_min = 59;
>> +             max_date.tm_sec = 59;
>> +
>
> Actually, this is wrong.
>
> If it is currently 1:23:45 on 22/09, you can go up to 1:23:44 on 22/10.
> trying to set a time after 1:23:45 will actually match on the same day
> instead of a month later.

Sorry, you are right, I initially did "current time - 1s", but then
for some reason I thought that in this case I'm also writing mday,
so I changed it to this. Obviously, if mday was also written, we
would be in the case here below...

>> +             t_max_date = rtc_tm_to_time64(&max_date);
>> +             t_alrm = rtc_tm_to_time64(&t->time);
>> +             if (t_alrm > t_max_date) {
>> +                     dev_err(dev,
>> +                             "Alarms can be up to one month in the future\n");
>> +                     return -EINVAL;
>> +             }
>> +     } else {
>> +             struct rtc_time max_date = now;
>> +             time64_t t_max_date;
>> +             time64_t t_alrm;
>> +             int max_mday;
>> +             bool allow_leap_day = false;
>> +
>> +             /*
>> +              * If it's the 28th of February and the next year is a leap
>> +              * year, allow to set alarms for the 29th of February.
>> +              */
>> +             if (now.tm_mon == 1) {
>> +                     max_mday = rtc_month_days(now.tm_mon, now.tm_year);
>> +                     if (now.tm_mday == max_mday)
>> +                             allow_leap_day = true;
>> +             }
>> +
>> +             max_date.tm_year += 1;
>> +             max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
>> +             if (max_date.tm_mday > max_mday || allow_leap_day)
>> +                     max_date.tm_mday = max_mday;
>> +
>> +             max_date.tm_hour = 23;
>> +             max_date.tm_min = 59;
>> +             max_date.tm_sec = 59;
>> +
>
> Ditto, 1:23:45 on 22/09/2016 can go up to 1:23:44 on 22/09/2017.

The errors are obvious now, I guess I should have looked at it
again with a fresh mind before submitting.

I think that if I remove all exceptional cases (leap years, longer
months) and go back to 'now - 1s' for the time, this should be fine.
I'll have a better look at it.

Gabriele

> Regards,
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3] rtc-cmos: Reject unsupported alarm values
  2016-09-02 19:55 ` [PATCH v2] " Gabriele Mazzotta
  2016-09-21 23:29   ` Alexandre Belloni
@ 2016-10-03 22:50   ` Gabriele Mazzotta
  2016-10-19  7:41     ` Alexandre Belloni
  1 sibling, 1 reply; 7+ messages in thread
From: Gabriele Mazzotta @ 2016-10-03 22:50 UTC (permalink / raw)
  To: alexandre.belloni, a.zummo; +Cc: rtc-linux, linux-kernel, Gabriele Mazzotta

Some platforms allows to specify the month and day of the month in
which an alarm should go off, some others the day of the month and
some others just the time.

Currently any given value is accepted by the driver and only the
supported fields are used to program the hardware. As consequence,
alarms are potentially programmed to go off in the wrong moment.

Fix this by rejecting any unsupported value.

Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/rtc/rtc-cmos.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 9bb14eb..2655432 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -328,14 +328,86 @@ static void cmos_irq_disable(struct cmos_rtc *cmos, unsigned char mask)
 	cmos_checkintr(cmos, rtc_control);
 }
 
+static int cmos_validate_alarm(struct device *dev, struct rtc_wkalrm *t)
+{
+	struct cmos_rtc *cmos = dev_get_drvdata(dev);
+	struct rtc_time now;
+
+	cmos_read_time(dev, &now);
+
+	if (!cmos->day_alrm) {
+		time64_t t_max_date;
+		time64_t t_alrm;
+
+		t_max_date = rtc_tm_to_time64(&now);
+		t_max_date += 24 * 60 * 60 - 1;
+		t_alrm = rtc_tm_to_time64(&t->time);
+		if (t_alrm > t_max_date) {
+			dev_err(dev,
+				"Alarms can be up to one day in the future\n");
+			return -EINVAL;
+		}
+	} else if (!cmos->mon_alrm) {
+		struct rtc_time max_date = now;
+		time64_t t_max_date;
+		time64_t t_alrm;
+		int max_mday;
+
+		if (max_date.tm_mon == 11) {
+			max_date.tm_mon = 0;
+			max_date.tm_year += 1;
+		} else {
+			max_date.tm_mon += 1;
+		}
+		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
+		if (max_date.tm_mday > max_mday)
+			max_date.tm_mday = max_mday;
+
+		t_max_date = rtc_tm_to_time64(&max_date);
+		t_max_date -= 1;
+		t_alrm = rtc_tm_to_time64(&t->time);
+		if (t_alrm > t_max_date) {
+			dev_err(dev,
+				"Alarms can be up to one month in the future\n");
+			return -EINVAL;
+		}
+	} else {
+		struct rtc_time max_date = now;
+		time64_t t_max_date;
+		time64_t t_alrm;
+		int max_mday;
+
+		max_date.tm_year += 1;
+		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
+		if (max_date.tm_mday > max_mday)
+			max_date.tm_mday = max_mday;
+
+		t_max_date = rtc_tm_to_time64(&max_date);
+		t_max_date -= 1;
+		t_alrm = rtc_tm_to_time64(&t->time);
+		if (t_alrm > t_max_date) {
+			dev_err(dev,
+				"Alarms can be up to one year in the future\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
 	unsigned char mon, mday, hrs, min, sec, rtc_control;
+	int ret;
 
 	if (!is_valid_irq(cmos->irq))
 		return -EIO;
 
+	ret = cmos_validate_alarm(dev, t);
+	if (ret < 0)
+		return ret;
+
 	mon = t->time.tm_mon + 1;
 	mday = t->time.tm_mday;
 	hrs = t->time.tm_hour;
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] rtc-cmos: Reject unsupported alarm values
  2016-10-03 22:50   ` [PATCH v3] " Gabriele Mazzotta
@ 2016-10-19  7:41     ` Alexandre Belloni
  0 siblings, 0 replies; 7+ messages in thread
From: Alexandre Belloni @ 2016-10-19  7:41 UTC (permalink / raw)
  To: Gabriele Mazzotta; +Cc: a.zummo, rtc-linux, linux-kernel

Hi,

On 04/10/2016 at 00:50:28 +0200, Gabriele Mazzotta wrote :
> Some platforms allows to specify the month and day of the month in
> which an alarm should go off, some others the day of the month and
> some others just the time.
> 
> Currently any given value is accepted by the driver and only the
> supported fields are used to program the hardware. As consequence,
> alarms are potentially programmed to go off in the wrong moment.
> 
> Fix this by rejecting any unsupported value.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
>  drivers/rtc/rtc-cmos.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 

Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-10-19 14:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-31 22:59 [PATCH] rtc-cmos: Reject unsupported alarm values Gabriele Mazzotta
2016-08-31 23:41 ` Gabriele Mazzotta
2016-09-02 19:55 ` [PATCH v2] " Gabriele Mazzotta
2016-09-21 23:29   ` Alexandre Belloni
2016-09-22 19:47     ` Gabriele Mazzotta
2016-10-03 22:50   ` [PATCH v3] " Gabriele Mazzotta
2016-10-19  7:41     ` Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).