Re: [PATCH v7 3/3] drm/xe/sysctrl: Add generic mailbox passthrough debugfs entry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/sysctrl: resplen=0 triggers kernel warning in mailbox debugfs
- [Low] drm/xe/sysctrl: debugfs write parsing fails with multiple spaces
--

commit f1b559774e8c577604cfb968a849e4f8ab4d694e
Author: Anoop Vijay <anoop.c.vijay@intel.com>

    drm/xe/sysctrl: Add generic mailbox passthrough debugfs entry
    
    Add a "mailbox" debugfs entry that allows any System Controller
    mailbox command to be issued for bring-up and debug. group and
    command are supplied by the caller as the first two tokens of every
    write, followed by optional per-command response timeout and
    response-length overrides and the payload bytes:

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> index 10675cd774ee1..ba904cb245124 100644
> --- a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
[ ... ]
> +static ssize_t xe_sysctrl_mailbox_write(struct file *file, const char __user *ubuf,
> +					size_t len, loff_t *offp)
> +{
[ ... ]
> +	if (token && strstarts(token, "timeout=")) {
> +		if (kstrtouint(token + strlen("timeout="), 0, &timeout_ms) ||
> +		    timeout_ms > XE_SYSCTRL_MB_MAX_TIMEOUT_MS) {
> +			xe_err(xe, "sysctrl: invalid mailbox timeout (max %u ms)\n",
> +			       XE_SYSCTRL_MB_MAX_TIMEOUT_MS);
> +			return -EINVAL;
> +		}
> +		token = strsep(&tmp, " \t\n");
> +	}
> +
> +	if (token && strstarts(token, "resplen=")) {

[Severity: Low]
Could the token parsing in xe_sysctrl_mailbox_write() fail if there are
multiple spaces between the timeout= and resplen= arguments?

Since strsep() is only called once after parsing the timeout argument,
multiple spaces would result in an empty token being evaluated here.

This would cause the strstarts(token, "resplen=") check to fail and
potentially treat the optional argument as payload data instead.

> +		unsigned int resplen;
> +
> +		if (kstrtouint(token + strlen("resplen="), 0, &resplen) ||
> +		    resplen > XE_SYSCTRL_MB_MAX_DATA_SIZE) {
> +			xe_err(xe, "sysctrl: invalid mailbox resplen (max %zu bytes)\n",
> +			       (size_t)XE_SYSCTRL_MB_MAX_DATA_SIZE);
> +			return -EINVAL;
> +		}
> +		resp_len = resplen;
> +		token = strsep(&tmp, " \t\n");
> +	}
[ ... ]
> +	scoped_guard(mutex, &entry->lock) {
> +		entry->group = group;
> +		entry->command = command;
> +		entry->timeout_ms = timeout_ms;
> +
> +		xe_sysctrl_create_command(&cmd, group, command, input_len ? input : NULL, input_len,
> +					  entry->response_buf, resp_len);

[Severity: Medium]
If a user writes a mailbox command with resplen=0 via debugfs, does this
introduce a regression that triggers a kernel warning?

The entry->response_buf array pointer is passed unconditionally as the
response buffer to xe_sysctrl_create_command().

This causes cmd.data_out to be non-NULL, while cmd.data_out_len is 0.

Later in xe_sysctrl_send_command(), this would fail the assertion:
xe_assert(xe, !cmd->data_out || cmd->data_out_len);

> +		cmd.timeout_ms = timeout_ms;
> +
> +		guard(xe_pm_runtime)(xe);
> +		status = xe_sysctrl_send_command(entry->sc, &cmd, &out_len);
> +		entry->status = status;
> +		entry->response_len = status ? 0 : out_len;
> +	}
> +
> +	return status ? status : len;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925135521.1804299-5-anoop.c.vijay@intel.com?part=3