diff mbox series

media: mediatek: vcodec: Consider vdecsys presence in reg range check

Message ID 20230725204043.569799-1-nfraprado@collabora.com (mailing list archive)
State New, archived
Headers show
Series media: mediatek: vcodec: Consider vdecsys presence in reg range check | expand

Commit Message

Nícolas F. R. A. Prado July 25, 2023, 8:40 p.m. UTC
Commit fe8a33978383 ("media: mediatek: vcodec: Read HW active status
from syscon") allowed the driver to read the VDEC_SYS io space from a
syscon instead of from the reg property when reg-names are supplied.
However as part of that change, a smatch warning was introduced:

drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c:142 mtk_vcodec_get_reg_bases() error: buffer overflow 'mtk_dec_reg_names' 11 <= 11

With a correct Devicetree, that is, one that follows the dt-binding, it
wouldn't be possible to trigger such a buffer overflow. Even so, update
the range validation of the reg property, so that the smatch warning is
fixed and if an incorrect Devicetree is ever supplied the code errors
out instead of causing memory corruption.

Reported-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Closes: https://lore.kernel.org/all/b5fd2dff-14a5-3ad8-9698-d1a50f4516fa@xs4all.nl
Fixes: fe8a33978383 ("media: mediatek: vcodec: Read HW active status from syscon")
Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno July 26, 2023, 7:01 a.m. UTC | #1
Il 25/07/23 22:40, Nícolas F. R. A. Prado ha scritto:
> Commit fe8a33978383 ("media: mediatek: vcodec: Read HW active status
> from syscon") allowed the driver to read the VDEC_SYS io space from a
> syscon instead of from the reg property when reg-names are supplied.
> However as part of that change, a smatch warning was introduced:
> 
> drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c:142 mtk_vcodec_get_reg_bases() error: buffer overflow 'mtk_dec_reg_names' 11 <= 11
> 
> With a correct Devicetree, that is, one that follows the dt-binding, it
> wouldn't be possible to trigger such a buffer overflow. Even so, update
> the range validation of the reg property, so that the smatch warning is
> fixed and if an incorrect Devicetree is ever supplied the code errors
> out instead of causing memory corruption.
> 
> Reported-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Closes: https://lore.kernel.org/all/b5fd2dff-14a5-3ad8-9698-d1a50f4516fa@xs4all.nl
> Fixes: fe8a33978383 ("media: mediatek: vcodec: Read HW active status from syscon")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> 
>   drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> index 742b6903d030..cd62b3f68072 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
> @@ -124,7 +124,8 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
>   	/* Sizeof(u32) * 4 bytes for each register base. */
>   	reg_num = of_property_count_elems_of_size(pdev->dev.of_node, "reg",
>   						  sizeof(u32) * 4);
> -	if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {
> +	if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE ||

You could also simplify this like

int num_max_vdec_regs;

....

num_max_vdec_regs = no_vdecsys_reg ?
		    ARRAY_SIZE(mtk_dec_reg_names) : NUM_MAX_VDEC_REG_BASE;

if (reg_num <= 0 || reg_num > num_max_vdec_regs) ....

I'd go for the proposed solution, as it looks better in my eyes, but it's
ultimately your choice and probably just a personal preference.

That said, if you want to keep this commit as it is, you still get my

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> +	    (!has_vdecsys_reg && reg_num > NUM_MAX_VDEC_REG_BASE - 1)) {
>   		dev_err(&pdev->dev, "Invalid register property size: %d\n", reg_num);
>   		return -EINVAL;
>   	}
diff mbox series

Patch

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
index 742b6903d030..cd62b3f68072 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_drv.c
@@ -124,7 +124,8 @@  static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
 	/* Sizeof(u32) * 4 bytes for each register base. */
 	reg_num = of_property_count_elems_of_size(pdev->dev.of_node, "reg",
 						  sizeof(u32) * 4);
-	if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {
+	if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE ||
+	    (!has_vdecsys_reg && reg_num > NUM_MAX_VDEC_REG_BASE - 1)) {
 		dev_err(&pdev->dev, "Invalid register property size: %d\n", reg_num);
 		return -EINVAL;
 	}