| Message ID | 20230830140850.17130-1-avromanov@salutedevices.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drivers: meson: sm: correct meson_sm_* API retval handling | expand |
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@salutedevices.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@linaro.org>
Hi, On Wed, 30 Aug 2023 17:08:50 +0300, 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. > > [...] Thanks, Applied to https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git (v6.7/drivers) [1/1] drivers: meson: sm: correct meson_sm_* API retval handling https://git.kernel.org/amlogic/c/0d423c4a78984dd02f6596d6fd9dd40446eec517 These changes has been applied on the intermediate git tree [1]. The v6.7/drivers branch will then be sent via a formal Pull Request to the Linux SoC maintainers for inclusion in their intermediate git branches in order to be sent to Linus during the next merge window, or sooner if it's a set of fixes. In the cases of fixes, those will be merged in the current release candidate kernel and as soon they appear on the Linux master branch they will be backported to the previous Stable and Long-Stable kernels [2]. The intermediate git branches are merged daily in the linux-next tree [3], people are encouraged testing these pre-release kernels and report issues on the relevant mailing-lists. If problems are discovered on those changes, please submit a signed-off-by revert patch followed by a corrective changeset. [1] https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git [2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
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);
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@salutedevices.com> --- drivers/firmware/meson/meson_sm.c | 20 +++++++++++++------- include/linux/firmware/meson/meson_sm.h | 2 +- 2 files changed, 14 insertions(+), 8 deletions(-)