Sashiko(locally) reports possiblity of division by zero and
out-of-bounds bitwise shift in trace_clock_update().

Although the clock update is untrusted, we should at least have some
basic checks to avoid the clock value getting out of sync if the host
is buggy.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
 arch/arm64/kvm/hyp/nvhe/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
index 32fc4313fe43..a7fc61976fd0 100644
--- a/arch/arm64/kvm/hyp/nvhe/clock.c
+++ b/arch/arm64/kvm/hyp/nvhe/clock.c
@@ -35,6 +35,9 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
 	struct clock_data *clock = &trace_clock_data;
 	u64 bank = clock->cur ^ 1;
 
+	if (!mult || shift >= 64)
+		return;
+
 	clock->data[bank].mult			= mult;
 	clock->data[bank].shift			= shift;
 	clock->data[bank].epoch_ns		= epoch_ns;
-- 
2.54.0.545.g6539524ca2-goog

On Thu, 30 Apr 2026 10:37:24 +0000, Mostafa Saleh wrote:
> Sashiko(locally) reports possiblity of division by zero and
> out-of-bounds bitwise shift in trace_clock_update().
> 
> Although the clock update is untrusted, we should at least have some
> basic checks to avoid the clock value getting out of sync if the host
> is buggy.
> 
> [...]

Applied to fixes, thanks!

[1/1] KVM: arm64: Harden clock for nvhe/pKVM
      commit: 9a624ea3f26f40c76bd2c7f77cde30659d42efbd

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.

On Thu, Apr 30, 2026 at 10:37:24AM +0000, Mostafa Saleh wrote:
> Sashiko(locally) reports possiblity of division by zero and
> out-of-bounds bitwise shift in trace_clock_update().
> 
> Although the clock update is untrusted, we should at least have some
> basic checks to avoid the clock value getting out of sync if the host
> is buggy.

I am not sure about the gain here. The host can still write values that will
make it out of sync anyway.

The timestamp is ultimately fed and read by the host.

> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/clock.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
> index 32fc4313fe43..a7fc61976fd0 100644
> --- a/arch/arm64/kvm/hyp/nvhe/clock.c
> +++ b/arch/arm64/kvm/hyp/nvhe/clock.c
> @@ -35,6 +35,9 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
>  	struct clock_data *clock = &trace_clock_data;
>  	u64 bank = clock->cur ^ 1;
>  
> +	if (!mult || shift >= 64)
> +		return;
> +
>  	clock->data[bank].mult			= mult;
>  	clock->data[bank].shift			= shift;
>  	clock->data[bank].epoch_ns		= epoch_ns;
> -- 
> 2.54.0.545.g6539524ca2-goog
>

On Wed, May 6, 2026 at 4:03 PM Vincent Donnefort <vdonnefort@google.com> wrote:
>
> On Thu, Apr 30, 2026 at 10:37:24AM +0000, Mostafa Saleh wrote:
> > Sashiko(locally) reports possiblity of division by zero and
> > out-of-bounds bitwise shift in trace_clock_update().
> >
> > Although the clock update is untrusted, we should at least have some
> > basic checks to avoid the clock value getting out of sync if the host
> > is buggy.
>
> I am not sure about the gain here. The host can still write values that will
> make it out of sync anyway.
>
> The timestamp is ultimately fed and read by the host.
>

This is not about having the clock in sync, but to avoid executing
undefined behavior, such as division by zero or large shifts in cases
if the host is buggy and not malicious.
That was reported by Sashiko
https://sashiko.dev/#/patchset/20260501111928.259252-1-smostafa%40google.com

Thanks,
Mostafa

> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  arch/arm64/kvm/hyp/nvhe/clock.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
> > index 32fc4313fe43..a7fc61976fd0 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/clock.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/clock.c
> > @@ -35,6 +35,9 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> >       struct clock_data *clock = &trace_clock_data;
> >       u64 bank = clock->cur ^ 1;
> >
> > +     if (!mult || shift >= 64)
> > +             return;
> > +
> >       clock->data[bank].mult                  = mult;
> >       clock->data[bank].shift                 = shift;
> >       clock->data[bank].epoch_ns              = epoch_ns;
> > --
> > 2.54.0.545.g6539524ca2-goog
> >

On Wed, May 06, 2026 at 04:10:20PM +0100, Mostafa Saleh wrote:
> On Wed, May 6, 2026 at 4:03 PM Vincent Donnefort <vdonnefort@google.com> wrote:
> >
> > On Thu, Apr 30, 2026 at 10:37:24AM +0000, Mostafa Saleh wrote:
> > > Sashiko(locally) reports possiblity of division by zero and
> > > out-of-bounds bitwise shift in trace_clock_update().
> > >
> > > Although the clock update is untrusted, we should at least have some
> > > basic checks to avoid the clock value getting out of sync if the host
> > > is buggy.

> >
> > I am not sure about the gain here. The host can still write values that will
> > make it out of sync anyway.
> >
> > The timestamp is ultimately fed and read by the host.
> >
> 
> This is not about having the clock in sync, but to avoid executing
> undefined behavior, such as division by zero or large shifts in cases
> if the host is buggy and not malicious.
> That was reported by Sashiko
> https://sashiko.dev/#/patchset/20260501111928.259252-1-smostafa%40google.com
> 
> Thanks,
> Mostafa

I would then reword that to make it clear it just prevents the host triggering
UB on the hypervisor. It doesn't really harden much, which is fine because that
data isn't relevant for the hypervisor.

Other than that:

Reviewed-by: Vincent Donnefort <vdonnefort@google.com>

> 
> > >
> > > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > > ---
> > >  arch/arm64/kvm/hyp/nvhe/clock.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/clock.c b/arch/arm64/kvm/hyp/nvhe/clock.c
> > > index 32fc4313fe43..a7fc61976fd0 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/clock.c
> > > +++ b/arch/arm64/kvm/hyp/nvhe/clock.c
> > > @@ -35,6 +35,9 @@ void trace_clock_update(u32 mult, u32 shift, u64 epoch_ns, u64 epoch_cyc)
> > >       struct clock_data *clock = &trace_clock_data;
> > >       u64 bank = clock->cur ^ 1;
> > >
> > > +     if (!mult || shift >= 64)
> > > +             return;
> > > +
> > >       clock->data[bank].mult                  = mult;
> > >       clock->data[bank].shift                 = shift;
> > >       clock->data[bank].epoch_ns              = epoch_ns;
> > > --
> > > 2.54.0.545.g6539524ca2-goog
> > >

On Wed, 06 May 2026 16:23:26 +0100,
Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> On Wed, May 06, 2026 at 04:10:20PM +0100, Mostafa Saleh wrote:
> > On Wed, May 6, 2026 at 4:03 PM Vincent Donnefort <vdonnefort@google.com> wrote:
> > >
> > > On Thu, Apr 30, 2026 at 10:37:24AM +0000, Mostafa Saleh wrote:
> > > > Sashiko(locally) reports possiblity of division by zero and
> > > > out-of-bounds bitwise shift in trace_clock_update().
> > > >
> > > > Although the clock update is untrusted, we should at least have some
> > > > basic checks to avoid the clock value getting out of sync if the host
> > > > is buggy.
> 
> > >
> > > I am not sure about the gain here. The host can still write values that will
> > > make it out of sync anyway.
> > >
> > > The timestamp is ultimately fed and read by the host.
> > >
> > 
> > This is not about having the clock in sync, but to avoid executing
> > undefined behavior, such as division by zero or large shifts in cases
> > if the host is buggy and not malicious.
> > That was reported by Sashiko
> > https://sashiko.dev/#/patchset/20260501111928.259252-1-smostafa%40google.com
> > 
> > Thanks,
> > Mostafa
> 
> I would then reword that to make it clear it just prevents the host triggering
> UB on the hypervisor. It doesn't really harden much, which is fine because that
> data isn't relevant for the hypervisor.

I can repaint the commit message when applying this.

> 
> Other than that:
> 
> Reviewed-by: Vincent Donnefort <vdonnefort@google.com>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.