Received-SPF: pass (google.com: domain of linux-kernel+bounces-96424-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <66a2c8b3-b1e8-4d2c-8a19-09e62099a2d7@linux.ibm.com>
Date: Fri, 8 Mar 2024 11:53:13 +1100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] rtc: fix uninitialized read of rtc_wkalrm.time
Content-Language: en-US
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: a.zummo@towertech.it, linux-rtc@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20231129073647.2624497-1-nicholas@linux.ibm.com>
 <20240229215850a1990100@mail.local>
From: Nicholas Miehlbradt <nicholas@linux.ibm.com>
In-Reply-To: <20240229215850a1990100@mail.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0


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
>>
>