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.
Signed-off-by: Nicholas Miehlbradt <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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/[email protected]/
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 <[email protected]>
>> ---
>> 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
>>
>
On 08/03/2024 11:53:13+1100, Nicholas Miehlbradt wrote:
>
>
> 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/[email protected]/
>
> 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?
Please do but I must say I don't really like "defensive" programming
just to please a static checker. There is no actual issue here, even if
the memory is not initialized, the result will never be incorrect and
the various checks that are being added everywhere are just bloating the
kernel, making it slower to boot and run. Now, we even have useless CVEs
that are assigned for this kind of issues...
Maybe it would be better to ensure the correctness of the result rather
than having a look at whether the memory was initialized or not.
> >
> > > Signed-off-by: Nicholas Miehlbradt <[email protected]>
> > > ---
> > > 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