Re: [PATCH 15/23] net: hpe: sl: add the NIC binding and logging helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] net: hpe: sl: missing lgrp ref and serialization in sl_media_cable_insert()
- [Medium] net: hpe: sl: format string truncation and injection in sl_log_msg_create()
- [Medium] net: hpe: sl: missing magic cases in sl_log_err_trace() and sl_log_warn_trace()
- [Low] net: hpe: sl: duplicate sl_media_cable_insert() and sl_media_cable_remove()
- [Low] net: hpe: sl: wrong NULL block error message in __sl_log_dynamic_dbg()
--

commit a4f50abb85b911c75a1fd3ef7f9989fb7a082030
Author: Doug Ledford <doug.ledford@hpe.com>

net: hpe: sl: add the NIC binding and logging helpers

This patch adds the logging helpers in sl_log.c and the NIC logical device
and media glue functions in nic/sl_ldev.c and nic/sl_media.c.

>     net: hpe: sl: add the NIC binding and logging helpers
>     
>     Add the glue that binds SL to a NIC: ASIC and CSR definitions, the NIC
>     logical device and media hooks, and the logging helpers.
>     
>     sl_asic_csrs.h is generated from the hardware register description and
>     is kept in that form; its integer types were converted to the kernel
>     types in the same pass as the other UAPI/header cleanups.
>     
>     checkpatch --strict (100 columns) still reports the following in this
>     patch. They need coordination with the internal repositories this code
>     is maintained in and will be fixed in V2:
>     - WARNING: LONG_LINE (18)
>     - CHECK: UNNECESSARY_PARENTHESES (16), COMPARISON_TO_NULL (3)
>     
>     Co-developed-by: Sandeep Bhoot <sandeep.bhoot@hpe.com>
>     Signed-off-by: Sandeep Bhoot <sandeep.bhoot@hpe.com>
>     Signed-off-by: Doug Ledford <doug.ledford@hpe.com>

[Severity: Low]
This isn't a bug, but the commit message mentions adding ASIC and CSR
definitions including sl_asic_csrs.h, whereas those were added in the
earlier commit 386ebd3c58a9 ("net: hpe: sl: add core headers"). Should this
description be updated to match the files in this patch?

> diff --git a/drivers/net/ethernet/hpe/sl/log/sl_log.c b/drivers/net/ethernet/hpe/sl/log/sl_log.c
> new file mode 100644
> index 0000000000000..64382f9b46bb3
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/log/sl_log.c
> @@ -0,0 +1,566 @@
[ ... ]
> +#define SL_LOG_MSG_LEN              150
> +#define SL_LOG_NULL_ID_FMT          "%c[-:--:-] ???         "
> +#define SL_LOG_DFLT_ID_FMT          "%c[-:--:-] 0x%08X  "
> +#define SL_LOG_LDEV_ID_FMT          "%c[%1u:--:-] ldev%1u       "
> +#define SL_LOG_LGRP_ID_FMT          "%c[%1u:%02u:-] %-12s"
> +#define SL_LOG_LINK_ID_FMT          "%c[%1u:%02u:%1u] %-12s"
> +#define SL_LOG_JACK_ID_FMT          "%c[-:--:-] jack%03u     "
> +#define SL_LOG_CONTEXT              (in_interrupt() ? '^' : ' ')
> +
> +static int remaining_chars(int idx)
> +{
> +	return max(SL_LOG_MSG_LEN - idx, 0);
> +}
> +
> +static void sl_log_msg_create(const char *level, void *ptr, char *msg,
> +			      const char *block, const char *name, const char *text)
> +{
[ ... ]
> +	idx = snprintf(msg, SL_LOG_MSG_LEN, "%s%s", level, module_name(THIS_MODULE));
> +	if (ptr != NULL) {
> +		switch (*((u32 *)ptr)) {
[ ... ]
> +		case SL_CTRL_LGRP_MAGIC:
> +			ctrl_lgrp = ptr;
> +			spin_lock_irqsave(&(ctrl_lgrp->log_lock), irq_flags);
> +			idx += snprintf(msg + idx, remaining_chars(idx), SL_LOG_LGRP_ID_FMT,
> +				SL_LOG_CONTEXT,
> +				ctrl_lgrp->ctrl_ldev->num, ctrl_lgrp->num,
> +				ctrl_lgrp->log_connect_id);
> +			spin_unlock_irqrestore(&(ctrl_lgrp->log_lock), irq_flags);
> +			break;
[ ... ]
> +		default:
> +			idx += snprintf(msg + idx, remaining_chars(idx), SL_LOG_DFLT_ID_FMT,
> +				SL_LOG_CONTEXT,
> +				*((u32 *)ptr));
> +			break;
> +		}
> +	} else {
> +		idx += snprintf(msg + idx, remaining_chars(idx), SL_LOG_NULL_ID_FMT,
> +			SL_LOG_CONTEXT);
> +	}
> +
> +	snprintf(msg + idx, remaining_chars(idx), " %5s-%-10s: %s\n", block, name, text);
> +}
> +
> +void sl_log(void *ptr, const char *level, const char *block,
> +	    const char *name, const char *text, ...)
> +{
> +	char    msg[SL_LOG_MSG_LEN + 1];
> +	va_list args;
[ ... ]
> +	sl_log_msg_create(level, ptr, msg, block, name, text);
> +
> +	va_start(args, text);
> +	vprintk(msg, args);
> +	va_end(args);
> +}

[Severity: Medium]
Can sl_log_msg_create() truncate caller format strings or allow format
specifiers in connect_id to be evaluated by vprintk()?

sl_log_msg_create() writes the log prefix (up to 49 bytes for level,
module_name(THIS_MODULE), SL_LOG_LGRP_ID_FMT or SL_LOG_LINK_ID_FMT, block,
and name) plus the unexpanded caller format string text and trailing newline
into the 150-byte msg buffer, and then sl_log(), sl_log_err_trace(),
sl_log_warn_trace(), sl_log_io_trace(), and __sl_log_dynamic_dbg() pass msg
directly as the format string to vprintk(msg, args).

Because only 100 bytes remain in msg for text and the newline, callers with
format strings over 100 bytes have their format string truncated before
vprintk() expands it (potentially mid-specifier), such as in
drivers/net/ethernet/hpe/sl/core/data/sl_core_data_llr.c:

    sl_core_data_llr_loop_time_get()
        sl_core_log_dbg(core_llr, LOG_NAME, ...);

and in drivers/net/ethernet/hpe/sl/core/hw/sl_core_hw_serdes_lane.c:

    sl_core_hw_serdes_lane_up_quality_check()
        sl_core_log_dbg(core_lgrp, LOG_NAME, ...);

In addition, ctrl_lgrp->log_connect_id, core_lgrp->log_connect_id, and
media_lgrp->connect_id are set at runtime via sl_lgrp_connect_id_set() (for
example from netdev_name(ndev) on NETDEV_CHANGENAME in cxi_netdev_event()),
so any percent character in connect_id is placed into msg and interpreted as
a format specifier by vprintk().

Would it be safer to format the message using printk with %pV (struct
va_format) instead of constructing a dynamic format string in msg?

> +
> +void sl_log_err_trace(void *ptr, const char *block,
> +		      const char *name, const char *text, ...)
> +{
[ ... ]
> +	if (ptr == NULL)
> +		return;
> +
> +	switch (*((u32 *)ptr)) {
> +	case SL_CTRL_LGRP_MAGIC:
[ ... ]
> +	case SL_MEDIA_LGRP_MAGIC:
> +		media_lgrp = ptr;
> +		rtn = sl_media_data_lgrp_is_err_trace_enabled(media_lgrp, &err_trace_enable);
> +		if (rtn)
> +			return;
> +		if (!err_trace_enable)
> +			return;
> +		break;
> +	default:
> +		return;
> +	}
[ ... ]
> +}
> +
> +void sl_log_warn_trace(void *ptr, const char *block,
> +		       const char *name, const char *text, ...)
> +{
[ ... ]
> +	if (ptr == NULL)
> +		return;
> +
> +	switch (*((u32 *)ptr)) {
> +	case SL_CTRL_LGRP_MAGIC:
[ ... ]
> +	case SL_MEDIA_LGRP_MAGIC:
> +		media_lgrp = ptr;
> +		rtn = sl_media_data_lgrp_is_warn_trace_enabled(media_lgrp, &warn_trace_enable);
> +		if (rtn)
> +			return;
> +		if (!warn_trace_enable)
> +			return;
> +		break;
> +	default:
> +		return;
> +	}

[Severity: Medium]
Are SL_MEDIA_JACK_MAGIC, SL_CTRL_LDEV_MAGIC, SL_LINK_MAGIC, and NULL
intentionally omitted from sl_log_err_trace() and sl_log_warn_trace()?

While sl_log_msg_create() handles SL_MEDIA_JACK_MAGIC, SL_CTRL_LDEV_MAGIC,
SL_LINK_MAGIC, and NULL pointers, the switch statements in
sl_log_err_trace() and sl_log_warn_trace() hit default: return (or return
early when ptr is NULL) for these types.

Across drivers/net/ethernet/hpe/sl/media/ and
drivers/net/ethernet/hpe/sl/ctrl/, callers pass struct sl_media_jack
pointers to sl_media_log_err_trace(), sl_media_log_warn_trace(), and
sl_ctrl_log_warn_trace() (for example in sl_media_jack_cable_insert(),
called by sl_media_cable_insert() in this patch), struct sl_ctrl_ldev
pointers in sl_ctrl_ldev.c, struct sl_link in sl_core_hw_an_lp.c, and NULL
pointers in ctrl/, causing all of those error and warning trace messages to
be silently dropped.

[ ... ]
> +void __sl_log_dynamic_dbg(struct _ddebug *desc, void *ptr, const char *block,
> +			  const char *name, const char *text, ...)
> +{
> +	char    msg[SL_LOG_MSG_LEN + 1];
> +	va_list args;
> +
> +	if (!desc) {
> +		pr_err("NULL desc\n");
> +		return;
> +	}
> +	if (!block) {
> +		pr_err("NULL desc\n");
> +		return;
> +	}

[Severity: Low]
Should the !block check in __sl_log_dynamic_dbg() log "NULL block\n" instead
of "NULL desc\n"?

[ ... ]
> diff --git a/drivers/net/ethernet/hpe/sl/nic/sl_media.c b/drivers/net/ethernet/hpe/sl/nic/sl_media.c
> new file mode 100644
> index 0000000000000..dddfaef0cb283
> --- /dev/null
> +++ b/drivers/net/ethernet/hpe/sl/nic/sl_media.c
> @@ -0,0 +1,53 @@
[ ... ]
> +int sl_media_cable_insert(struct sl_lgrp *lgrp, u8 *eeprom_page0, u8 *eeprom_page1, u32 flags)
> +{
> +	int rtn;
> +
> +	rtn = sl_lgrp_check(lgrp);
> +	if (rtn) {
> +		sl_log_err(NULL, LOG_BLOCK, LOG_NAME, "cable insert fail");
> +		return rtn;
> +	}
> +	if (!eeprom_page0) {
> +		sl_log_err(NULL, LOG_BLOCK, LOG_NAME, "NULL eeprom");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * hardcoding 0 for now since there is only one jack. Might change in future
> +	 */
> +	return sl_media_jack_cable_insert(lgrp->ldev_num, lgrp->num, 0, eeprom_page0,
> +					  eeprom_page1, flags);
> +}
> +EXPORT_SYMBOL(sl_media_cable_insert);
> +
> +int sl_media_cable_remove(struct sl_lgrp *lgrp)
> +{
> +	int rtn;
> +
> +	rtn = sl_lgrp_check(lgrp);
> +	if (rtn) {
> +		sl_log_err(NULL, LOG_BLOCK, LOG_NAME, "cable remove fail");
> +		return rtn;
> +	}
> +
> +	/*
> +	 * hardcoding 0 for now since there is only one jack. Might change in future
> +	 */
> +	return sl_media_jack_cable_remove(lgrp->ldev_num, lgrp->num, 0);
> +}
> +EXPORT_SYMBOL(sl_media_cable_remove);

[Severity: High]
Can sl_media_cable_insert() and sl_media_cable_remove() race with link
teardown or concurrent cable scans, leading to use-after-free on media_jack
and media_lgrp or a NULL pointer dereference in
sl_sysfs_media_speeds_create()?

Here, sl_media_cable_insert() and sl_media_cable_remove() only validate the
static lgrps[ldev_num][lgrp_num] handle via sl_lgrp_check(lgrp) and then
call sl_media_jack_cable_insert() and sl_media_jack_cable_remove() without
pinning ctrl_lgrp (via sl_ctrl_lgrp_get() and
sl_ctrl_lgrp_kref_get_unless_zero()) or serializing cable state transitions.

In drivers/net/ethernet/hpe/ss1/cass_core.c, cass_remove() and the
cass_probe() error path invoke teardown before disabling microcontroller
interrupts and flushing hw->uc_attn1_wq:

    cass_remove()
        hw->link_ops->link_fini(hw) -> cass_sl_link_fini()
            cass_sl_media_unconfig() -> sl_media_cable_remove()
            cass_sl_exit()
                sl_lgrp_del() -> sl_ctrl_lgrp_del()
                sl_ldev_del() -> sl_ctrl_ldev_del()
        cass_unregister_uc(hw)

While attn1_worker() is running:

    attn1_worker()
        cass_cable_scan()
            cass_link_headshell_insert()
                cass_sl_media_config()
                    sl_media_cable_insert()
                        sl_media_jack_cable_insert()
                            sl_media_data_jack_cable_soft_reset() (sleeps 3.3s)

because ctrl_lgrp is not pinned, sl_ctrl_lgrp_del() and sl_ctrl_ldev_del()
proceed immediately to sl_sysfs_media_delete(), sl_media_lgrp_del() (freeing
media_lgrp), and sl_media_data_jack_del() (freeing media_jack). When
sl_media_jack_cable_insert() wakes from msleep(), it writes to freed
media_jack memory and calls sl_sysfs_media_speeds_create(ldev_num,
lgrp_num), which dereferences a NULL media_lgrp at
media_lgrp->speeds_kobj_init.

Similarly, during cass_sbl_link_start(), cass_cable_scan() runs on the probe
thread after cass_register_uc() has already enabled ATT1_QSFP_INSERT and
ATT1_QSFP_REMOVE interrupts on hw->uc_attn1_wq, allowing attn1_worker() to
enter sl_media_cable_insert() or sl_media_cable_remove() concurrently and
corrupt media_lgrp->speeds_kobj[] initialization and teardown.

[Severity: Low]
Earlier commit 44ba5c7fee3ca ("net: hpe: sl: add the logical group, link,
MAC, and LLR APIs") added drivers/net/ethernet/hpe/sl/sl_media_nic.c with
placeholder definitions and EXPORT_SYMBOL() declarations for
sl_media_cable_insert() and sl_media_cable_remove():

drivers/net/ethernet/hpe/sl/sl_media_nic.c:
    int sl_media_cable_insert(struct sl_lgrp *lgrp, u8 *eeprom_page0, u8 *eeprom_page1, u32 flags)
    {
        /* placeholder for future functionality */
        return 0;
    }
    EXPORT_SYMBOL(sl_media_cable_insert);

    int sl_media_cable_remove(struct sl_lgrp *lgrp)
    {
        /* placeholder for future functionality */
        return 0;
    }
    EXPORT_SYMBOL(sl_media_cable_remove);

Should drivers/net/ethernet/hpe/sl/sl_media_nic.c be removed now that
drivers/net/ethernet/hpe/sl/nic/sl_media.c provides the real implementations
of these functions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-cxi-netdev-submission-v1-0-8c2c391cc283@hpe.com?part=15