On Thu, Mar 14, 2024 at 09:35:20PM +0800, Peng Fan (OSS) wrote:
> +enum scmi_pinctrl_protocol_cmd {
> + PINCTRL_ATTRIBUTES = 0x3,
> + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> + PINCTRL_CONFIG_GET = 0x5,
> + PINCTRL_CONFIG_SET = 0x6,
> + PINCTRL_FUNCTION_SELECT = 0x7,
PINCTRL_FUNCTION_SELECT was removed from the spec so the other cmds were
renumbered. I'm still going through and reviewing this file. I'll
hopefully be done tomorrow.
> + PINCTRL_REQUEST = 0x8,
> + PINCTRL_RELEASE = 0x9,
> + PINCTRL_NAME_GET = 0xa,
> + PINCTRL_SET_PERMISSIONS = 0xb
> +};
> +
[ snip ]
> +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> + enum scmi_pinctrl_selector_type type,
> + u32 selector, char *name,
> + unsigned int *n_elems)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_pinctrl_attributes *tx;
> + struct scmi_resp_pinctrl_attributes *rx;
> +
> + if (!name)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, selector, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> + sizeof(*rx), &t);
> + if (ret)
> + return ret;
> +
> + tx = t->tx.buf;
> + rx = t->rx.buf;
> + tx->identifier = cpu_to_le32(selector);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + if (!ret) {
> + if (n_elems)
> + *n_elems = NUM_ELEMS(rx->attributes);
> +
> + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> + }
> +
> + ph->xops->xfer_put(ph, t);
> +
> + /*
> + * If supported overwrite short name with the extended one;
> + * on error just carry on and use already provided short name.
> + */
> + if (!ret && EXT_NAME_FLAG(rx->attributes))
^^^^
Dereferencing "rx" after the ph->xops->xfer_put() is a use after free
(racy).
> + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> + (u32 *)&type, name,
> + SCMI_MAX_STR_SIZE);
> + return ret;
> +}
[ snip ]
> +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx), 0, &t);
Missing error check.
if (ret)
return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
> +
> +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
> + u32 pin)
> +{
> + return scmi_pinctrl_request(ph, pin, PIN_TYPE);
> +}
> +
> +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> + u32 identifier,
> + enum scmi_pinctrl_selector_type type)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_request *tx;
> +
> + if (type == FUNCTION_TYPE)
> + return -EINVAL;
> +
> + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> + if (ret)
> + return ret;
> +
> + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
if (ret)
return ret;
> +
> + tx = t->tx.buf;
> + tx->identifier = cpu_to_le32(identifier);
> + tx->flags = cpu_to_le32(type);
> +
> + ret = ph->xops->do_xfer(ph, t);
> + ph->xops->xfer_put(ph, t);
> +
> + return ret;
> +}
[ snip ]
> +enum scmi_pinctrl_conf_type {
> + SCMI_PIN_NONE = 0x0,
This is SCMI_PIN_DEFAULT now.
> + SCMI_PIN_BIAS_BUS_HOLD = 0x1,
> + SCMI_PIN_BIAS_DISABLE = 0x2,
> + SCMI_PIN_BIAS_HIGH_IMPEDANCE = 0x3,
> + SCMI_PIN_BIAS_PULL_UP = 0x4,
> + SCMI_PIN_BIAS_PULL_DEFAULT = 0x5,
> + SCMI_PIN_BIAS_PULL_DOWN = 0x6,
> + SCMI_PIN_DRIVE_OPEN_DRAIN = 0x7,
> + SCMI_PIN_DRIVE_OPEN_SOURCE = 0x8,
> + SCMI_PIN_DRIVE_PUSH_PULL = 0x9,
> + SCMI_PIN_DRIVE_STRENGTH = 0xA,
> + SCMI_PIN_INPUT_DEBOUNCE = 0xB,
> + SCMI_PIN_INPUT_MODE = 0xC,
> + SCMI_PIN_PULL_MODE = 0xD,
> + SCMI_PIN_INPUT_VALUE = 0xE,
> + SCMI_PIN_INPUT_SCHMITT = 0xF,
> + SCMI_PIN_LOW_POWER_MODE = 0x10,
> + SCMI_PIN_OUTPUT_MODE = 0x11,
> + SCMI_PIN_OUTPUT_VALUE = 0x12,
> + SCMI_PIN_POWER_SOURCE = 0x13,
> + SCMI_PIN_SLEW_RATE = 0x20,
^^^^
This is a decimal vs hex bug. It should 0x14. I think this enum would
be more readable in decimal anyway.
> + SCMI_PIN_OEM_START = 0xC0,
> + SCMI_PIN_OEM_END = 0xFF,
> +};
But I'm still trying to review the code so I'll probably respond more
tomorrow.
regards,
dan carpenter
