diff mbox series

[next] rtc: msc313: Fix unintentional sign extension issue on left shift of a u16

Message ID 20210928123906.988813-1-colin.king@canonical.com (mailing list archive)
State New, archived
Headers show
Series [next] rtc: msc313: Fix unintentional sign extension issue on left shift of a u16 | expand

Commit Message

Colin King Sept. 28, 2021, 12:39 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Shifting the u16 value returned by readw by 16 bits to the left
will be promoted to a 32 bit signed int and then sign-extended
to an unsigned long. If the top bit of the readw is set then
the shifted value will be sign extended and the top 32 bits of
the result will be set.

Fixes: be7d9c9161b9 ("rtc: Add support for the MSTAR MSC313 RTC")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/rtc/rtc-msc313.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Palmer Sept. 28, 2021, 1:31 p.m. UTC | #1 
Hi Colin,

On Tue, 28 Sept 2021 at 21:39, Colin King <colin.king@canonical.com> wrote:
>Shifting the u16 value returned by readw by 16 bits to the left
>will be promoted to a 32 bit signed int and then sign-extended
>to an unsigned long. If the top bit of the readw is set then
>the shifted value will be sign extended and the top 32 bits of
>the result will be set.

Ah,.. C is fun in all the wrong places. :)
These chips are full of 32bit registers that are split into two 16
registers 4 bytes apart when seen from the ARM CPU so we probably have
this same mistake in a few other places.

A similar pattern is used a bit later on in the same file to read the counter:

seconds = readw(priv->rtc_base + REG_RTC_CNT_VAL_L)
| (readw(priv->rtc_base + REG_RTC_CNT_VAL_H) << 16);

I guess it works at the moment because the top bit won't be set until 2038.

Thanks,

Daniel
Colin King Sept. 28, 2021, 1:34 p.m. UTC | #2 
On 28/09/2021 14:31, Daniel Palmer wrote:
> Hi Colin,
> 
> On Tue, 28 Sept 2021 at 21:39, Colin King <colin.king@canonical.com> wrote:
>> Shifting the u16 value returned by readw by 16 bits to the left
>> will be promoted to a 32 bit signed int and then sign-extended
>> to an unsigned long. If the top bit of the readw is set then
>> the shifted value will be sign extended and the top 32 bits of
>> the result will be set.
> 
> Ah,.. C is fun in all the wrong places. :)
> These chips are full of 32bit registers that are split into two 16
> registers 4 bytes apart when seen from the ARM CPU so we probably have
> this same mistake in a few other places.
> 
> A similar pattern is used a bit later on in the same file to read the counter:
> 
> seconds = readw(priv->rtc_base + REG_RTC_CNT_VAL_L)
> | (readw(priv->rtc_base + REG_RTC_CNT_VAL_H) << 16);

Ah, I missed that one! I'll send a V2.

> 
> I guess it works at the moment because the top bit won't be set until 2038.

I hope to be retired by then, but I guess fixing it up before 2038 is a
good idea ;-)

> 
> Thanks,
> 
> Daniel
>
Romain Perier Sept. 28, 2021, 1:54 p.m. UTC | #3 
Hi,

Le mar. 28 sept. 2021 à 15:31, Daniel Palmer <daniel@0x0f.com> a écrit :
>
> Hi Colin,
>
> On Tue, 28 Sept 2021 at 21:39, Colin King <colin.king@canonical.com> wrote:
> >Shifting the u16 value returned by readw by 16 bits to the left
> >will be promoted to a 32 bit signed int and then sign-extended
> >to an unsigned long. If the top bit of the readw is set then
> >the shifted value will be sign extended and the top 32 bits of
> >the result will be set.

Good catch !

>
> Ah,.. C is fun in all the wrong places. :)
> These chips are full of 32bit registers that are split into two 16
> registers 4 bytes apart when seen from the ARM CPU so we probably have
> this same mistake in a few other places.
>
> A similar pattern is used a bit later on in the same file to read the counter:
>
> seconds = readw(priv->rtc_base + REG_RTC_CNT_VAL_L)
> | (readw(priv->rtc_base + REG_RTC_CNT_VAL_H) << 16);
>
> I guess it works at the moment because the top bit won't be set until 2038.

The crazy stuff being, I ran rtctest from selftests and rtc-range (1)
that tests a variety
of dates including 2038 and 2106 for example. Both tests passed :) (probably
because *this case* specifically did not happen while running the test)

1. https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c

Thanks,
Regards,
Romain
Daniel Palmer Sept. 29, 2021, 12:56 p.m. UTC | #4 
Hi Romain,

On Tue, 28 Sept 2021 at 22:55, Romain Perier <romain.perier@gmail.com> wrote:
>
> Hi,
>
> Le mar. 28 sept. 2021 à 15:31, Daniel Palmer <daniel@0x0f.com> a écrit :
> The crazy stuff being, I ran rtctest from selftests and rtc-range (1)
> that tests a variety
> of dates including 2038 and 2106 for example. Both tests passed :) (probably
> because *this case* specifically did not happen while running the test)

I suspect it works because for reading the time because seconds is a
u32 not unsigned long like the other functions.
So if the high word of the register is read, is promoted to a wider
type and sign extended it doesn't actually matter because it gets
truncated to 32 bits so the sign extended part is gone.

Cheers,

Daniel
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-msc313.c b/drivers/rtc/rtc-msc313.c
index 5f178d29cfd8..7430244ad867 100644
--- a/drivers/rtc/rtc-msc313.c
+++ b/drivers/rtc/rtc-msc313.c
@@ -53,7 +53,7 @@  static int msc313_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	unsigned long seconds;
 
 	seconds = readw(priv->rtc_base + REG_RTC_MATCH_VAL_L)
-			| (readw(priv->rtc_base + REG_RTC_MATCH_VAL_H) << 16);
+			| ((unsigned long)readw(priv->rtc_base + REG_RTC_MATCH_VAL_H) << 16);
 
 	rtc_time64_to_tm(seconds, &alarm->time);