Re: [PATCH v4 4/4] 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] Unprotected state modifications to the shared xe_sysctrl_debugfs_entry in the mailbox debugfs read/write paths.
- [Low] The mailbox debugfs write function fails to skip empty tokens when parsing the group and command IDs, causing valid user input with leading or consecutive whitespace to be rejected.
--

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

drm/xe/sysctrl: Add generic mailbox passthrough debugfs entry
    
Adds a generic mailbox debugfs entry that allows any System Controller
mailbox command to be issued for bring-up and debug.

> diff --git a/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c b/drivers/gpu/drm/xe/xe_sysctrl_debugfs.c
> index 258479d605cbe..e5bc0a5dc31f8 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)
> +{
[ ... ]
> +	tmp = kbuf;
> +
> +	token = strsep(&tmp, " \t\n");
> +	if (!token || kstrtoul(token, 0, &val) || val > 0xFF) {

[Severity: Low]
Does this code reject valid user input with leading spaces? When parsing the
group ID in xe_sysctrl_mailbox_write(), if a user executes a command like
"echo ' 1 2' > mailbox", strsep() will return an empty string for the leading
space. This causes kstrtoul() to fail and reject the command.

> +		xe_err(xe, "sysctrl: invalid mailbox group id\n");
> +		return -EINVAL;
> +	}
> +	group = (u8)val;
> +
> +	token = strsep(&tmp, " \t\n");
> +	if (!token || kstrtoul(token, 0, &val) || val > 0xFF) {

[Severity: Low]
The same parsing issue appears here for the command ID when there are
consecutive spaces. Can these be updated to skip empty tokens, similar to how
the payload byte parsing loop handles them?

> +		xe_err(xe, "sysctrl: invalid mailbox command id\n");
> +		return -EINVAL;
> +	}
[ ... ]
> +	entry->group = group;
> +	entry->command = command;
> +
> +	xe_sysctrl_create_command(&cmd, group, command, input_len ? input : NULL, input_len,
> +				  entry->response_buf, XE_SYSCTRL_MB_MAX_MESSAGE_SIZE);
> +
> +	guard(xe_pm_runtime)(xe);
> +	entry->status = xe_sysctrl_send_command(entry->sc, &cmd, &out_len);
> +	entry->response_len = entry->status ? 0 : out_len;

[Severity: Medium]
Are these shared state modifications protected against concurrent access? If
two processes concurrently execute xe_sysctrl_mailbox_write() on the mailbox
debugfs file, could it cause a data race? This might corrupt the shared
xe_sysctrl_debugfs_entry state and mix command arguments or response data
from different threads.

> +	return entry->status ? entry->status : len;
> +}
> +
> +static int xe_sysctrl_mailbox_show(struct seq_file *m, void *data)
> +{
> +	struct xe_sysctrl_debugfs_entry *entry = m->private;
> +	size_t i;
[ ... ]
> +	if (entry->response_len) {
> +		seq_puts(m, "Response data:\n");
> +		for (i = 0; i < entry->response_len; i++) {
> +			if (i && (i % 16) == 0)
> +				seq_putc(m, '\n');
> +			seq_printf(m, "%02x ", entry->response_buf[i]);
> +		}

[Severity: Medium]
Similarly, this shared state is read locklessly in xe_sysctrl_mailbox_show().
Could a concurrent write operation interleave during this loop and cause
inconsistent or mixed-up data to be displayed in the debugfs output?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903140340.83500-6-anoop.c.vijay@intel.com?part=4