linux-kernel - Re: [PATCH] rtc: fix uninitialized read of rtc

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: <66a2c8b3-b1e8-4d2c-8a19-09e62099a2d7@linux.ibm.com>
Date: Fri, 8 Mar 2024 11:53:13 +1100
From: Nicholas Miehlbradt <nicholas@...ux.ibm.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: a.zummo@...ertech.it, linux-rtc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rtc: fix uninitialized read of rtc_wkalrm.time



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@...ux.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
>>
>