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

[Simon Horman <horms@kernel.org>]

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.

> 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?

> +
> +			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.

> +	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?

>  		tspec->tv_nsec = mul_u64_u64_shr(frac_sec, NSEC_PER_SEC, 64);
>  		tspec->tv_sec += le64_to_cpu(st->clk->time_sec);
>  
>  		if (!tai_adjust(st->clk, &tspec->tv_sec))
>  			return -EINVAL;
>  
> +		/*
> +		 * Capture clock state inside the seq_count loop for a
> +		 * consistent snapshot with the timestamp. The attrs path
> +		 * reports it to userspace via the status field; the legacy
> +		 * path saves it for the UNRELIABLE check after the loop.
> +		 */