Re: [PATCH net-next v4 1/5] net: dsa: mv88e6xxx: only time stamp PTPv2 frames

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing PTP header linearization leading to potential out-of-bounds memory accesses.
- [High] The MV88E6XXX_HWTSTAMP_ENABLED bit flag is used as an ad-hoc synchronization mechanism for hardware reconfiguration, failing to protect against in-flight readers.
--

--- Patch [1]: [PATCH net-next v4 1/5] net: dsa: mv88e6xxx: only time stamp PTPv2 frames ---
commit 105747c72e976395df1d9d44672056bd86952a7c
Author: Luke Howard <lukeh@padl.com>

net: dsa: mv88e6xxx: only time stamp PTPv2 frames

Marvell switches only document support for IEEE 1588v2, not for PTPv1
(IEEE 1588-2022). Reject PTPv1 frames in mv88e6xxx_should_tstamp().

> diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.c b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> index 6e6472a3b75ad..e4326165e1896 100644
> --- a/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> +++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.c
> @@ -216,6 +216,10 @@ static struct ptp_header *mv88e6xxx_should_tstamp(struct mv88e6xxx_chip *chip,
>  	if (!chip->info->ptp_support)
>  		return NULL;
>  
> +	/* Marvell switches do not support IEEE 1588-2002 (PTPv1). */
> +	if ((type & PTP_CLASS_VMASK) != PTP_CLASS_V2)
> +		return NULL;
> +
>  	hdr = ptp_parse_header(skb, type);
>  	if (!hdr)
>  		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does ptp_parse_header()
guarantee that the returned header is in the linear region of the skb?

ptp_parse_header() bounds checks against skb->data + skb->len, which includes
paged data. If the header spans into paged fragments, callers that directly
dereference the returned header without first calling pskb_may_pull() could
cause an out-of-bounds read.

This happens in the RX path when calling is_pdelay_resp():

drivers/net/dsa/mv88e6xxx/hwtstamp.c:is_pdelay_resp() {
    return (hdr->tsmt & 0xf) == 3;
}

It also happens in seq_match():

drivers/net/dsa/mv88e6xxx/hwtstamp.c:seq_match() {
    ...
    hdr = ptp_parse_header(skb, type);
    return ts_seqid == ntohs(hdr->sequence_id);
}

and in the TX path:

drivers/net/dsa/mv88e6xxx/hwtstamp.c:mv88e6xxx_port_txtstamp() {
    ...
    ps->tx_seq_id = be16_to_cpu(hdr->sequence_id);
}

Could this lead to kernel oopses or garbage reads?


[Severity: High]
This is another pre-existing issue, but does using the
MV88E6XXX_HWTSTAMP_ENABLED bit flag provide adequate synchronization?

The configuration path in mv88e6xxx_set_hwtstamp_config() clears this bit
to act as an ad-hoc lock:

drivers/net/dsa/mv88e6xxx/hwtstamp.c:mv88e6xxx_set_hwtstamp_config() {
    /* Prevent the TX/RX paths from trying to interact with the
     * timestamp hardware while we reconfigure it.
     */
    clear_bit_unlock(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state);
    ...
}

And the TX/RX paths check it here in mv88e6xxx_should_tstamp(), just below
the diff hunk:

drivers/net/dsa/mv88e6xxx/hwtstamp.c:mv88e6xxx_should_tstamp() {
    ...
    if (!test_bit(MV88E6XXX_HWTSTAMP_ENABLED, &ps->state))
        return NULL;
    ...
}

If an in-flight reader in the TX/RX path checks the flag, gets preempted,
and resumes after the configuration thread alters the hardware state,
couldn't it queue timestamp requests to disabled hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mv88e6xxx-ptp-fixes-v4-0-10b4020155fc@padl.com?part=1