Re: [PATCH v6 net-next 4/7] ptp: ptp_vmclock: Implement attributes ioctls
From: Simon Horman <horms@kernel.org>
Date: 2026-08-04 12:37:48
Also in:
linux-doc, linux-kselftest
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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)
[ ... ]
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ + 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.
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
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. + */