Re: [PATCH] rtc: fix uninitialized read of rtc_wkalrm.time

[Nicholas Miehlbradt <nicholas@linux.ibm.com>]

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@linux.ibm.com/

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@linux.ibm.com>
>> ---
>>   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
>>
>