| Message ID | 20231129073647.2624497-1-nicholas@linux.ibm.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | rtc: fix uninitialized read of rtc_wkalrm.time | expand |
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 >
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 >> >
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/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? 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 <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 > > > > >
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; }
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 <nicholas@linux.ibm.com> --- drivers/rtc/interface.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)