Re: [PATCH v10 07/14] dpll: sit9531x: add support to get and set priority on input pins

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dpll: sit9531x: unreachable priority table rollback leaves HW corrupted
- [High] dpll: sit9531x: UAF and list corruption in sit9531x_dpll_input_pin_prio_set
--

commit b6317b5bb6f30f209960f172a34c584a3034f1e4
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: sit9531x: add support to get and set priority on input pins

This commit adds DPLL netlink priority get and set callbacks for the
sit9531x device. It manages the hardware priority table mapping inputs to
slots, caching hardware state for fast read-backs.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 84c1f86e6d4cf..ac184c93258cb 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -629,9 +683,34 @@ static int sit9531x_prio_table_commit(struct sit9531x_dev *sitdev, u8 pll_idx,
>  
>  	written = SIT9531X_PRIO_MAX_SLOTS;
>  
> +	if (rc && written) {
> +		/*
> +		 * Put the slots that did reach the device back the way they
> +		 * were.  Latching a table that is neither the previous order
> +		 * nor the requested one hands the reference selection loop
> +		 * a priority list nobody asked for.  The cache is the table
> +		 * as last read, which is what those slots held.
> +		 */
> +		for (slot = 0; slot < written; slot += 2) {
> +			u8 old;
> +
> +			old = sit9531x_prio_slot_set(0, slot,
> +						     chan->prio_srcs[slot]);
> +			old = sit9531x_prio_slot_set(old, slot + 1,
> +						     chan->prio_srcs[slot + 1]);

[Severity: High]
Does this bypass the rollback block entirely? Every failure path for
sit9531x_write_u8() and sit9531x_read_u8() just prior to this in
sit9531x_prio_table_commit() jumps directly to the 'commit:' label, skipping
the 'if (rc && written)' check. Will this leave the priority table partially
updated in hardware on a transient I2C error?

Additionally, if this rollback code were somehow reached and 'slot' reached
10, would accessing chan->prio_srcs[slot + 1] result in an out-of-bounds
read at index 11?

[ ... ]
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 4437c95f578f8..56a8213dee077 100644
> --- a/drivers/dpll/sit9531x/dpll.c
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> +static int
> +sit9531x_dpll_input_pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
> +				 const struct dpll_device *dpll,
> +				 void *dpll_priv, u32 prio,
> +				 struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	mutex_lock(&sitdev->multiop_lock);
> +	rc = sit9531x_input_prio_set(sitdev, sitdpll->id,
> +				     sit9531x_input_hw_src(dpin->id),
> +				     (u8)prio);
> +	if (!rc) {
> +		list_for_each_entry(sibling, &sitdpll->pins, list) {

[Severity: High]
Is this list iteration over sitdpll->pins safe against concurrent
modification? It appears sit9531x_dpll_pins_unregister() can remove
elements and free them via kfree() without holding sitdev->multiop_lock.

If a userspace netlink request sets pin priority concurrently with device
removal (e.g., driver unbind), could this unprotected iteration lead to a
use-after-free and list corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921201108.42676-1-arouhi@sitime.com?part=7