Re: [PATCH v6 net-next 4/7] ptp: ptp_vmclock: Implement attributes ioctls

[Arthur Kiyanovski <akiyano@amazon.com>]

On 2026-08-04 13:37:39+01:00, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev/#/patchset/20260729233740.16516-2-akiyano@amazon.com?part=4
> ---
> --- Patch [4]: [PATCH v6 net-next 4/7] ptp: ptp_vmclock: Implement attributes ioctls ---
> commit 9ab73b649349967c6e3befcbed4c125d48bd8c43
> Author: Arthur Kiyanovski <akiyano@amazon.com>
> 
> ptp: ptp_vmclock: Implement attributes ioctls
> 
> Implement the gettimexattrs64 and getcrosststampattrs callbacks in the
> ptp_vmclock driver to provide clock quality attributes through the new
> PTP_SYS_OFFSET_EXTENDED_ATTRS and PTP_SYS_OFFSET_PRECISE_ATTRS ioctls.
> 

Thanks Simon, and thanks for the AI review. Responses inline.

> > diff --git a/drivers/ptp/ptp_vmclock.c b/drivers/ptp/ptp_vmclock.c
> > index eebdcd5ebc080..fe26123ad940d 100644
> > --- a/drivers/ptp/ptp_vmclock.c
> > +++ b/drivers/ptp/ptp_vmclock.c
> > @@ -95,13 +106,111 @@ static bool tai_adjust(struct vmclock_abi *clk, uint64_t *sec)
> 
> [ ... ]
> 
> > +static void vmclock_populate_ptp_attributes(struct vmclock_state *st,
> > +					    struct ptp_clock_attrs *att,
> > +					    uint64_t delta)
> > +{
> > +	uint64_t maxerror_ns = UINT_MAX;
> > +
> > +	if (!att)
> > +		return;
> > +
> > +	/* Only calculate if the base error is flagged as valid
> > +	 * by the hypervisor.
> > +	 */
> > +	if (VMCLOCK_FIELD_PRESENT(st->clk, time_maxerror_nanosec) &&
> > +	    (le64_to_cpu(st->clk->flags) & VMCLOCK_FLAG_TIME_MAXERROR_VALID)) {
> > +		maxerror_ns = le64_to_cpu(st->clk->time_maxerror_nanosec);
> > +
> > +		/* If frequency error is also valid, accumulate it
> > +		 * over the delta.
> > +		 */
> > +		if (VMCLOCK_FIELD_PRESENT(st->clk, counter_period_maxerror_rate_frac_sec) &&
> > +		    (le64_to_cpu(st->clk->flags) & VMCLOCK_FLAG_PERIOD_MAXERROR_VALID)) {
> > +			uint64_t maxerror_rate, err_hi, err_frac, growth_ns;
> > +
> > +			if (st->clk->counter_period_shift >= 128) {
> > +				maxerror_ns = U64_MAX;
> > +				goto saturate;
> > +			}
> > +
> > +			maxerror_rate = le64_to_cpu(st->clk->counter_period_maxerror_rate_frac_sec);
> > +			err_frac = mul_u64_u64_shr_add_u64(&err_hi, delta,
> > +							   maxerror_rate,
> > +							   st->clk->counter_period_shift,
> > +							   0);
> 
> [Severity: High]
> Could a malicious hypervisor modify counter_period_shift concurrently?
> 
> The counter_period_shift field is read directly from shared memory twice,
> first for the bounds check and then as an argument to
> mul_u64_u64_shr_add_u64(). If the value changes between these reads, it
> could bypass the bounds check and cause an out-of-bounds shift.
> 
> Would it be better to read it once using READ_ONCE() to ensure the bounds
> check operates on the exact value passed to the math helper?
> 

Good point that the >= 128 guard here is inconsistent with the rest of
the driver: counter_period_shift is read from the shared page and passed
to the same helper on the main timestamp path (vmclock_get_crosststamp())
with no bounds check, so this field has always been treated as trusted
via the seq_count snapshot. Adding a guard (or READ_ONCE()) to only the
new attrs path is the wrong place to change that posture.

For v7 I'll drop the guard so both paths read the field identically.
With the guard gone the attrs path reads counter_period_shift once, so
the divergent double-read the review flagged no longer exists.


> > +
> > +			if (err_hi >= U64_MAX / NSEC_PER_SEC) {
> > +				maxerror_ns = U64_MAX;
> > +				goto saturate;
> > +			}
> > +
> > +			growth_ns = (err_hi * NSEC_PER_SEC) +
> > +				    mul_u64_u64_shr(err_frac, NSEC_PER_SEC, 64);
> > +
> > +			/* Guard against overflow */
> > +			if (U64_MAX - growth_ns < maxerror_ns)
> > +				maxerror_ns = U64_MAX;
> > +			else
> > +				maxerror_ns += growth_ns;
> > +		}
> > +	}
> > +
> > +saturate:
> > +	/* PTP UAPI error_bound is 32-bit nanoseconds */
> > +	att->error_bound = (maxerror_ns > UINT_MAX) ?
> > +		UINT_MAX : (uint32_t)maxerror_ns;
> > +	att->valid |= PTP_ATTRS_VALID_ERROR_BOUND;
> 
> [Severity: Medium]
> Does this unconditionally tell userspace that the error bound is valid, even
> if the hypervisor flagged time_maxerror_nanosec as invalid?
> 
> If VMCLOCK_FLAG_TIME_MAXERROR_VALID is clear, maxerror_ns remains UINT_MAX
> and PTP_ATTRS_VALID_ERROR_BOUND is still set. Userspace might interpret the
> resulting UINT_MAX as a legitimate worst-case error bound rather than an
> unsupported attribute.
> 

Agreed, that's misleading. This will be fixed in v7:
PTP_ATTRS_VALID_ERROR_BOUND is only set when the hypervisor flags the
base error valid (VMCLOCK_FLAG_TIME_MAXERROR_VALID); otherwise the bit
stays clear so userspace sees "not provided" rather than a UINT_MAX that
looks like a real ~4.29 s bound.

> > +	att->timescale = vmclock_get_ptp_timescale(st->clk->time_type);
> > +	/* tai_adjust() already converted UTC to TAI before we're called */
> > +	if (st->clk->time_type == VMCLOCK_TIME_UTC)
> > +		att->timescale = PTP_TIMESCALE_TAI;
> > +	att->valid |= PTP_ATTRS_VALID_TIMESCALE;
> > +	att->status = vmclock_get_ptp_status(st->clk->clock_status);
> > +	att->valid |= PTP_ATTRS_VALID_STATUS;
> > +}
> > +
> >  static int vmclock_get_crosststamp(struct vmclock_state *st,
> >  				   struct ptp_system_timestamp *sts,
> >  				   struct system_counterval_t *system_counter,
> 
> [ ... ]
> 
> >  		delta = cycle - le64_to_cpu(st->clk->counter_value);
> >  
> >  		frac_sec = mul_u64_u64_shr_add_u64(&tspec->tv_sec, delta,
> >  						   le64_to_cpu(st->clk->counter_period_frac_sec),
> >  						   st->clk->counter_period_shift,
> >  						   le64_to_cpu(st->clk->time_frac_sec));
> 
> [Severity: High]
> This is a pre-existing issue, but can counter_period_shift be greater than
> or equal to 128 here?
> 
> Since counter_period_shift is controlled by the hypervisor and passed directly
> to mul_u64_u64_shr_add_u64() without bounds checking, an invalid value could
> cause an out-of-bounds bitwise shift and undefined behavior:
> 
> mul_u64_u64_shr_add_u64() {
>     ...
>     res >>= shift;
>     ...
> }
> 
> Given that vmclock_populate_ptp_attributes() actively defends against
> invalid shift values, should we add a similar bounds check here to prevent
> a potential kernel panic?

As noted, this is pre-existing: it predates this series -- it's in the
original vmclock driver, and my v6 attrs helper was the only
guarded use. Dropping that guard (above) keeps the new path consistent
with the existing one rather than diverging from it.

If the shift should actually be bounds-checked, that's a fix to the
existing driver and better handled on its own rather than folded into
this attributes series. I'll defer to David Woodhouse as vmclock
maintainer on whether that hardening is worth doing separately.

Thanks,
Arthur