Received-SPF: pass (google.com: domain of linux-kernel+bounces-87609-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 29 Feb 2024 22:58:50 +0100
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Nicholas Miehlbradt <nicholas@linux.ibm.com>
Cc: a.zummo@towertech.it, linux-rtc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rtc: fix uninitialized read of rtc_wkalrm.time
Message-ID: <20240229215850a1990100@mail.local>
References: <20231129073647.2624497-1-nicholas@linux.ibm.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231129073647.2624497-1-nicholas@linux.ibm.com>

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? :)

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

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com