On 1/3/2024 8:58 am, Alexandre Belloni wrote:
Hello,
On 29/11/2023 07:36:47+0000, Nicholas Miehlbradt wrote:
If either of the first two branches of the if statement in
rtc_read_alarm_internal are taken the fields of alarm->time are not
initialized but are subsequently read by the call to rtc_tm_to_time64.
Refactor so that the time field is only read if the final branch of the
if statment which initializes the field is taken.
While the problem description is correct, the solution is not because
you have no guarantee that the fields have been initialized if
->read_alarm returns a value different from 0
So, instead of avoiding the conversion unless the final branch is taken,
it should be avoided as long as err != 0.
But, I'm also wondering whether there is actually an issue. mktime64
can be fed whatever value without bugging out and the value of err will
be part of the trace so userspace knows that it shouldn't trust the
value.
So, what is the actual issue? :)
Thank you for your feedback.
I found this issue during my implementation of KMSAN for powerpc. The
goal with this patch is to eliminate use of undefined memory which leads
to undefined behaviour, I should have made this more clear in my
original message. You can find the KMSAN patch series here:
https://lore.kernel.org/linuxppc-dev/20231214055539.9420-1-nicholas@xxxxxxxxxxxxx/
I can make the changes suggested and fold this patch into the next
version of my KMSAN series if that would help to add context as to why I
am submitting this patch?
Signed-off-by: Nicholas Miehlbradt <nicholas@xxxxxxxxxxxxx>
---
drivers/rtc/interface.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 1b63111cdda2..f40e76d2fe2b 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -179,6 +179,7 @@ static int rtc_read_alarm_internal(struct rtc_device *rtc,
struct rtc_wkalrm *alarm)
{
int err;
+ time64_t trace_time = -1;
err = mutex_lock_interruptible(&rtc->ops_lock);
if (err)
@@ -201,11 +202,12 @@ static int rtc_read_alarm_internal(struct rtc_device *rtc,
alarm->time.tm_yday = -1;
alarm->time.tm_isdst = -1;
err = rtc->ops->read_alarm(rtc->dev.parent, alarm);
+ trace_time = rtc_tm_to_time64(&alarm->time);
}
mutex_unlock(&rtc->ops_lock);
- trace_rtc_read_alarm(rtc_tm_to_time64(&alarm->time), err);
+ trace_rtc_read_alarm(trace_time, err);
return err;
}
--
2.37.2
