linux-kernel - Re: [PATCH v1] drivers: meson: sm: correct meson_sm

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: <a020e7a9-d91a-4a15-86ef-fdb05c78ae3f@linaro.org>
Date:   Fri, 8 Sep 2023 14:41:48 +0200
From:   neil.armstrong@...aro.org
To:     Alexey Romanov <avromanov@...utedevices.com>, khilman@...libre.com,
        jbrunet@...libre.com, miles.chen@...iatek.com,
        martin.blumenstingl@...glemail.com, narmstrong@...libre.com
Cc:     kernel@...rdevices.ru, linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] drivers: meson: sm: correct meson_sm_* API retval
 handling

On 30/08/2023 16:08, Alexey Romanov wrote:
> 1. Following the ARM SMC32 calling convention, the return value
> from secure monitor is a 32-bit signed integer. This patch changes
> the type of the return value of the function meson_sm_call().
> 
> 2. Now, when meson_sm_call() returns a 32-bit signed integer, we need
> to ensure that this value is not negative. It is important to check
> that the return value is not negative in both the meson_sm_call_read()
> and meson_sm_call_write() functions.
> 
> 3. Add a comment explaining why it is necessary to check if the SMC
> return value is equal to 0 in the function meson_sm_call_read().
> It is not obvious when reading this code.
> 
> Signed-off-by: Alexey Romanov <avromanov@...utedevices.com>
> ---
>   drivers/firmware/meson/meson_sm.c       | 20 +++++++++++++-------
>   include/linux/firmware/meson/meson_sm.h |  2 +-
>   2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index 798bcdb05d84..27a8cd4747f8 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c
> @@ -67,7 +67,7 @@ static u32 meson_sm_get_cmd(const struct meson_sm_chip *chip,
>   	return cmd->smc_id;
>   }
>   
> -static u32 __meson_sm_call(u32 cmd, u32 arg0, u32 arg1, u32 arg2,
> +static s32 __meson_sm_call(u32 cmd, u32 arg0, u32 arg1, u32 arg2,
>   			   u32 arg3, u32 arg4)
>   {
>   	struct arm_smccc_res res;
> @@ -102,9 +102,10 @@ static void __iomem *meson_sm_map_shmem(u32 cmd_shmem, unsigned int size)
>    * Return:	0 on success, a negative value on error
>    */
>   int meson_sm_call(struct meson_sm_firmware *fw, unsigned int cmd_index,
> -		  u32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> +		  s32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
>   {
> -	u32 cmd, lret;
> +	u32 cmd;
> +	s32 lret;
>   
>   	if (!fw->chip)
>   		return -ENOENT;
> @@ -143,7 +144,7 @@ int meson_sm_call_read(struct meson_sm_firmware *fw, void *buffer,
>   		       unsigned int bsize, unsigned int cmd_index, u32 arg0,
>   		       u32 arg1, u32 arg2, u32 arg3, u32 arg4)
>   {
> -	u32 size;
> +	s32 size;
>   	int ret;
>   
>   	if (!fw->chip)
> @@ -158,11 +159,16 @@ int meson_sm_call_read(struct meson_sm_firmware *fw, void *buffer,
>   	if (meson_sm_call(fw, cmd_index, &size, arg0, arg1, arg2, arg3, arg4) < 0)
>   		return -EINVAL;
>   
> -	if (size > bsize)
> +	if (size < 0 || size > bsize)
>   		return -EINVAL;
>   
>   	ret = size;
>   
> +	/* In some cases (for example GET_CHIP_ID command),
> +	 * SMC doesn't return the number of bytes read, even
> +	 * though the bytes were actually read into sm_shmem_out.
> +	 * So this check is needed.
> +	 */
>   	if (!size)
>   		size = bsize;
>   
> @@ -192,7 +198,7 @@ int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
>   			unsigned int size, unsigned int cmd_index, u32 arg0,
>   			u32 arg1, u32 arg2, u32 arg3, u32 arg4)
>   {
> -	u32 written;
> +	s32 written;
>   
>   	if (!fw->chip)
>   		return -ENOENT;
> @@ -208,7 +214,7 @@ int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
>   	if (meson_sm_call(fw, cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)
>   		return -EINVAL;
>   
> -	if (!written)
> +	if (written <= 0 || written > size)
>   		return -EINVAL;
>   
>   	return written;
> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
> index 95b0da2326a9..8eaf8922ab02 100644
> --- a/include/linux/firmware/meson/meson_sm.h
> +++ b/include/linux/firmware/meson/meson_sm.h
> @@ -19,7 +19,7 @@ enum {
>   struct meson_sm_firmware;
>   
>   int meson_sm_call(struct meson_sm_firmware *fw, unsigned int cmd_index,
> -		  u32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
> +		  s32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
>   int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
>   			unsigned int b_size, unsigned int cmd_index, u32 arg0,
>   			u32 arg1, u32 arg2, u32 arg3, u32 arg4);

Reviewed-by: Neil Armstrong <neil.armstrong@...aro.org>