Re: [PATCH net 1/2] dpll: fix NULL deref in dpll_device_ops() during teardown race

[Jakub Kicinski <kuba@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.
---
dpll: fix NULL deref in dpll_device_ops() during teardown race

This commit modifies dpll_device_ops() and dpll_priv() to return NULL during
a teardown race when the registration list is empty. It also adds a check in
dpll_cmd_pin_get_one() to drop the notification if the first referenced dpll
is unregistered.

> In-tree doit/dumpit callers of dpll_priv() / dpll_device_ops() obtain
> their dpll via dpll_pre_doit() -> dpll_device_get_by_id(), which only
> returns DPLL_REGISTERED devices; dpll_device_unregister() clears that
> mark only after observing list_empty(&dpll->registration_list), so
> those callers cannot see the new NULL return. The only path that can
> is dpll_cmd_pin_get_one() reached from dpll_pin_event_send(), gated
> here.

Is this description accurate for pin operations?

Looking at doit and dumpit paths for pins, they appear to use
dpll_pin_pre_doit() which relies on dpll_pin_available(). That function
allows the operation if at least one dpll referencing the pin is registered.

If a pin references multiple dpll devices, and one is registered while
another is half-dead (being unregistered), couldn't a user trigger a panic by
executing a pin dump or configuration command during a driver unload?

> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index ea63602637866..b73d718e8a56d 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -709,6 +709,12 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
>  	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>  	ASSERT_NOT_NULL(ref);
>  
> +	/* The first dpll the pin references may be torn down while still
> +	 * pinned by foreign-driver refs; drop the notification cleanly.
> +	 */
> +	if (!dpll_device_ops(ref->dpll))
> +		return -ENODEV;
> +

Will returning -ENODEV here break the netlink pin dump globally?

Since dpll_cmd_pin_get_one() is called inside the dumpit loop of
dpll_nl_pin_get_dumpit(), returning an error aborts the entire dump process.
A single pin in this transient state seems to break the pin dump operation
for all pins.

Also, for notifications sent via dpll_pin_event_send(), this return value
silently cancels and frees the netlink message. If the pin is shared with
other active dplls (which might be later in the xarray), won't their valid
state change notifications be permanently lost? Should the code skip
unregistered dplls instead of unconditionally aborting the operation?

Furthermore, does this check fully prevent the NULL pointer dereference?

This only validates the first dpll reference returned by
dpll_xa_ref_dpll_first(). If the first dpll is registered but a subsequent
dpll is half-dead (its registration list is empty), this check passes.

Execution would then proceed to functions like dpll_msg_add_pin_dplls() or
netlink set operations like dpll_pin_phase_adj_set() which iterate over
all dpll references in pin->dpll_refs. For the half-dead dpll, dpll_priv(dpll)
now evaluates to NULL, which is passed to driver callbacks
like ops->state_on_dpll_get that might unconditionally dereference it
(for example, ice_dpll_pin_state_get() uses struct ice_pf *pf = d->pf),
causing a kernel panic.
-- 
pw-bot: cr