linux-kernel - Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

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] [day] [month] [year] [list]

Message-ID: <6b6a163b-be75-4003-a618-f0e928a6d114@oss.qualcomm.com>
Date: Sat, 26 Apr 2025 11:49:14 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Sricharan R <quic_srichara@...cinc.com>, jassisinghbrar@...il.com,
        robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, andersson@...nel.org,
        konradybcio@...nel.org, manivannan.sadhasivam@...aro.org,
        dmitry.baryshkov@...aro.org
Subject: Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox
 driver

On 3/27/25 7:17 PM, Sricharan R wrote:
> From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
> 
> This mailbox facilitates the communication between the TMEL server
> subsystem (Trust Management Engine Lite) and the TMEL client
> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
> authentication, enable/disable efuses, crypto services etc. Each client in
> the SoC has its own block of message RAM and IRQ for communication with the
> TMEL SS. The protocol used to communicate in the message RAM is known as
> Qualcomm Messaging Protocol (QMP).
> 
> Remote proc driver subscribes to this mailbox and uses the
> mbox_send_message to use TMEL to securely authenticate/teardown the images.
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@...cinc.com>
> ---

[...]

> +
> +#define QMP_NUM_CHANS		0x1

Quantities make more sense in decimal, but since this is effectively
a single-use value, you can put in the '1' literal in num_chans and use
devm_kzalloc instead of devm_kcalloc in the other use

> +#define QMP_TOUT_MS		1000

"TIMEOUT"

> +#define QMP_CTRL_DATA_SIZE	4
> +#define QMP_MAX_PKT_SIZE	0x18

This is very handwavy, please structurize all data that comes in and
out of the mailbox.

> +#define QMP_UCORE_DESC_OFFSET	0x1000
> +#define QMP_SEND_TIMEOUT	30000

Please include the unit in the macro name - although 30s is quite a
timeout for a couple bytes..

[...]

> +#define QMP_HW_MBOX_SIZE		32
> +#define QMP_MBOX_RSV_SIZE		4
> +#define QMP_MBOX_IPC_PACKET_SIZE	(QMP_HW_MBOX_SIZE - QMP_CTRL_DATA_SIZE - QMP_MBOX_RSV_SIZE)
> +#define QMP_MBOX_IPC_MAX_PARAMS		5
> +
> +#define QMP_MAX_PARAM_IN_PARAM_ID	14
> +#define QMP_PARAM_CNT_FOR_OUTBUF	3
> +#define QMP_SRAM_IPC_MAX_PARAMS		(QMP_MAX_PARAM_IN_PARAM_ID * QMP_PARAM_CNT_FOR_OUTBUF)
> +#define QMP_SRAM_IPC_MAX_BUF_SIZE	(QMP_SRAM_IPC_MAX_PARAMS * sizeof(u32))

These should be expressed in terms of structures and sizeof() instead,
as well

> +
> +#define TMEL_ERROR_GENERIC		(0x1u)
> +#define TMEL_ERROR_NOT_SUPPORTED	(0x2u)
> +#define TMEL_ERROR_BAD_PARAMETER	(0x3u)
> +#define TMEL_ERROR_BAD_MESSAGE		(0x4u)
> +#define TMEL_ERROR_BAD_ADDRESS		(0x5u)
> +#define TMEL_ERROR_TMELCOM_FAILURE	(0x6u)
> +#define TMEL_ERROR_TMEL_BUSY		(0x7u)

Oh I didn't notice this during the first review.. I assume these are
returned by the mbox. Please create a dictionary such as:

u32 tmel_error_dict[] = {
	[TMEL_ERROR_GENERIC] = EINVAL,
	[TMEL_ERROR_NOT_SUPPORTED] = EOPNOTSUPP
	...
};

that we can then plug into the function down below that currently does
error ? -EINVAL : 0

Konrad