Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Use-After-Free of Pin Properties and Frequency Arrays
- [High] State Mismatch / Logic Flaw in Pin Getters
- [High] Destructive Priority Overwrite in Hardware Table
- [High] Use-After-Free on Probe Error Path Due to Missing DPLL Unregistration
- [Medium] Silent Failure on Manual Pin Connection Request
- [Medium] Missing SYSREF/SYNCB/Pulser Features
- [Medium] Hardware Routing Loop via INTSYNC
--
--- Patch [3]: [PATCH net-next v6 3/3] dpll: add SiTime SiT9531x DPLL clock driver ---
commit 841a91aff3f30b8a2e2bee8952633134d7bbdee2
Author: Ali Rouhi <[email protected]>
Subject: dpll: add SiTime SiT9531x DPLL clock driver
Summary: This commit adds a DPLL subsystem driver for the SiTime SiT95316
and SiT95317 clock generators. It supports hardware state monitoring, priority
management, and frequency/phase configuration.
> - SYSREF/SYNCB/Pulser output mode control
[Severity: Medium]
Is this functionality fully exposed to userspace? The commit message mentions
support for SYSREF/SYNCB/Pulser modes, but looking at the driver,
sit9531x_output_pulse_ctrl_set() only appears to be called with a hardcoded
default during embedded sync setup. It looks like the other modes might not
be reachable.
> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 0000000000000..711ab16b0897a
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> @@ -596,32 +596,25 @@ int sit9531x_input_prio_set(struct sit9531x_dev *sitdev, u8 pll_idx,
>
> usleep_range(10000, 12000);
>
> /*
> * Write the requested slot on Page 1, then fill the remaining
> * slots with the same source, matching the procedure script's
> * backfill behaviour.
> */
> for (slot = prio; slot < SIT9531X_PRIO_MAX_SLOTS; slot++) {
> reg = sit9531x_prio_reg(pll_idx, slot);
>
> rc = sit9531x_read_u8(sitdev, reg, &val);
> if (rc)
> goto commit;
>
> val = sit9531x_prio_slot_set(val, slot, input_idx);
[Severity: High]
Does this loop unintentionally wipe out fallback configurations? When userspace
sets a priority, this loop fills not only the requested slot but also all
lower-priority slots with the exact same input source. This appears to destroy
any other fallback inputs the user might have configured in the hardware table.
[ ... ]
> @@ -2496,25 +2496,15 @@ int sit9531x_dev_start(struct sit9531x_dev *sitdev)
> if (rc)
> return rc;
>
> list_for_each_entry(sitdpll, &sitdev->dplls, list) {
> rc = sit9531x_dpll_register(sitdpll);
> if (rc) {
> dev_err_probe(sitdev->dev, rc,
> "Failed to register DPLL%u\n",
> sitdpll->id);
> return rc;
> }
> }
[ ... ]
> @@ -2825,18 +2815,10 @@ static void sit9531x_dpll_list_free(struct sit9531x_dev *sitdev)
> {
> struct sit9531x_dpll *sitdpll, *next;
>
> list_for_each_entry_safe(sitdpll, next, &sitdev->dplls, list) {
> list_del(&sitdpll->list);
> sit9531x_dpll_free(sitdpll);
> }
> }
[Severity: High]
Could this error path leave the DPLL subsystem with dangling pointers? If
sit9531x_dpll_register() succeeds for the first DPLL but fails for the next,
sit9531x_dev_start() returns an error without unwinding the already
registered DPLLs. The error handler in sit9531x_devm_dpll_init() then calls
sit9531x_dpll_list_free() to free the backing memory directly, bypassing
dpll_device_unregister() and dpll_pin_unregister().
> diff --git a/drivers/dpll/sit9531x/dpll.c b/drivers/dpll/sit9531x/dpll.c
> index 0000000000000..364f870ac3041
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/dpll.c
[ ... ]
> @@ -259,32 +259,25 @@ static int
> sit9531x_dpll_input_pin_state_on_dpll_get(const struct dpll_pin *pin,
> void *pin_priv,
> const struct dpll_device *dpll,
> void *dpll_priv,
> enum dpll_pin_state *state,
> struct netlink_ext_ack *extack)
> {
[ ... ]
> if (chan->locked && !chan->inner_lol &&
> chan->selected_ref == dpin->id)
> *state = DPLL_PIN_STATE_CONNECTED;
> /* In auto mode, enabled inputs with good signal are selectable */
> else if (!chan->mode && ref->enabled && !ref->los && !ref->oof)
> *state = DPLL_PIN_STATE_SELECTABLE;
> else
> *state = DPLL_PIN_STATE_DISCONNECTED;
[Severity: High]
Might this logic report an incorrect routing state? The ref->enabled flag
represents a global physical state for the pin. If DPLL A requests an input
and enables the receiver, DPLL B will also see ref->enabled as true and
report the pin as DPLL_PIN_STATE_SELECTABLE, even if that pin isn't in DPLL
B's priority table.
Additionally, if a signal is lost (ref->los becomes true), this drops the
state to DPLL_PIN_STATE_DISCONNECTED. Does the DPLL API expect degraded pins
to still show as logically connected or selectable rather than entirely
disconnected?
[ ... ]
> @@ -326,32 +319,25 @@ static int
> sit9531x_dpll_input_pin_state_on_dpll_set(const struct dpll_pin *pin,
> void *pin_priv,
> const struct dpll_device *dpll,
> void *dpll_priv,
> enum dpll_pin_state state,
> struct netlink_ext_ack *extack)
> {
[ ... ]
> switch (state) {
> case DPLL_PIN_STATE_DISCONNECTED:
> rc = sit9531x_input_prio_remove(sitdev, sitdpll->id, hw_src);
> if (rc)
> break;
> ref->pll_mask &= ~pll_bit;
> if (ref->pll_mask)
> rc = 0; /* another DPLL still uses this input */
> else
> rc = sit9531x_input_disable(sitdev, dpin->id);
> break;
> case DPLL_PIN_STATE_SELECTABLE:
> case DPLL_PIN_STATE_CONNECTED:
> rc = sit9531x_input_enable(sitdev, dpin->id);
> if (rc)
> break;
> rc = sit9531x_input_prio_add(sitdev, sitdpll->id, hw_src);
[Severity: Medium]
Should DPLL_PIN_STATE_CONNECTED enforce a manual lock here? It appears to
fall through to the exact same logic as DPLL_PIN_STATE_SELECTABLE, adding
the input to the priority table but leaving the hardware in automatic
selection mode instead of forcing a connection.
[ ... ]
> @@ -651,32 +637,25 @@ static int
> sit9531x_dpll_intsync_src_state_on_dpll_set(const struct dpll_pin *pin,
> void *pin_priv,
> const struct dpll_device *dpll,
> void *dpll_priv,
> enum dpll_pin_state state,
> struct netlink_ext_ack *extack)
> {
[ ... ]
> switch (state) {
> case DPLL_PIN_STATE_CONNECTED:
> if (sitdev->intsync_src == sitdpll->id)
> break;
> if (sitdev->intsync_src >= 0) {
> NL_SET_ERR_MSG(extack,
> "INTSYNC is already sourced by another PLL");
> rc = -EBUSY;
> break;
> }
> rc = sit9531x_intsync_enable(sitdev, sitdpll->id);
[Severity: Medium]
Is there a protection missing here against hardware routing loops? While the
destination getter correctly prevents adding INTSYNC to a PLL's priority
table if that PLL is already the source, this function doesn't seem to check
if the PLL already has INTSYNC as a destination before making it the source.
Could userspace configure a PLL to source from INTSYNC and then drive INTSYNC
with the same PLL?
> diff --git a/drivers/dpll/sit9531x/prop.c b/drivers/dpll/sit9531x/prop.c
> index 0000000000000..8a0105c8c6472
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/prop.c
[ ... ]
> @@ -357,10 +357,8 @@ void sit9531x_pin_props_put(struct sit9531x_pin_props *props)
> {
> kfree(props->dpll_props.freq_supported);
>
> if (props->fwnode)
> fwnode_handle_put(props->fwnode);
>
> kfree(props);
> }
[Severity: Critical]
Will this lead to a use-after-free when userspace queries the pin properties?
The sit9531x_pin_props_get() function dynamically allocates both the props
structure and the freq_supported array. When passed into dpll_pin_get()
in sit9531x_dpll_pin_register(), the DPLL core retains those pointers.
Since sit9531x_pin_props_put() immediately frees these allocations on the
success path, it looks like the DPLL core might be left holding dangling
pointers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.