linux-kernel - Re: [PATCH v6 06/11] misc: amd-sbi: Add support for AMD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <c9138dd3-6a09-47a2-a8fe-716c8894042e@app.fastmail.com>
Date: Mon, 24 Mar 2025 16:40:12 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Akshay Gupta" <akshay.gupta@....com>, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: "Guenter Roeck" <linux@...ck-us.net>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>, shyam-sundar.s-k@....com,
 gautham.shenoy@....com, "Mario Limonciello" <mario.limonciello@....com>,
 naveenkrishna.chatradhi@....com, anand.umarji@....com
Subject: Re: [PATCH v6 06/11] misc: amd-sbi: Add support for AMD_SBI IOCTL

On Mon, Mar 24, 2025, at 15:58, Akshay Gupta wrote:
> ---
> Changes since v5:
> - Address review comment
>  - Address Arnd comments
>  - Add check for cmd in ioctl

I think you missed one of my comments.

> +static long sbrmi_ioctl(struct file *fp, unsigned int cmd, unsigned 
> long arg)
> +{
> +	struct apml_message msg = { 0 };
> +	struct sbrmi_data *data;
> +	bool read = false;
> +	int ret;
> +
> +	if (cmd != SBRMI_IOCTL_CMD)
> +		return -ENOTTY;
> +
> +	if (_IOC_SIZE(cmd) != sizeof(msg))
> +		return -EINVAL;

You are checking the 'cmd' argument to the function now, which
is good. There is no need to also check _IOC_SIZE, since
that is implied by the definition.
rue;

> +
> +	switch (msg.cmd) {
> +	case 0 ... 0x999:
> +		/* Mailbox protocol */
> +		ret = rmi_mailbox_xfer(data, &msg);
> +		break;

What is however missing here is a specific check for the
individual commands: I don't think it's a good idea to
treat all 2458 mailbox commands the same way and just
pass them through unfiltered here, and I would also not
pass the specific 'cmd' as part of a multiplexer structure.

Instead, I think there should be a separate ioctl command
for each thing you can do with the mailbox. From the existing
driver it appears that these are the commands currently
supported:

enum sbrmi_msg_id {
        SBRMI_READ_PKG_PWR_CONSUMPTION = 0x1,
        SBRMI_WRITE_PKG_PWR_LIMIT,
        SBRMI_READ_PKG_PWR_LIMIT,
        SBRMI_READ_PKG_MAX_PWR_LIMIT,
};

which is just the first four out of the 2458, and clearly small
enough to justify one ioctl command each. I don't know what
the command specific arguments would look like, so it's
possible you may also want to define a separate structure
for each one.

       Arnd