[PATCH v3] media: mtk-vpu: avoid unaligned access to DTCM buffer.

[PATCH v3] media: mtk-vpu: avoid unaligned access to DTCM buffer.

[Hsin-Yi Wang]

struct vpu_run *run in vpu_init_ipi_handler() is an ioremapped DTCM (Data
Tightly Coupled Memory) buffer shared with AP.  It's not able to do
unaligned access. Otherwise kernel would crash due to unable to handle
kernel paging request.

struct vpu_run {
	u32 signaled;
	char fw_ver[VPU_FW_VER_LEN];
	unsigned int	dec_capability;
	unsigned int	enc_capability;
	wait_queue_head_t wq;
};

fw_ver starts at 4 byte boundary. If system enables
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, strscpy() will do
read_word_at_a_time(), which tries to read 8-byte: *(unsigned long *)addr

Copy the string by memcpy_fromio() for this buffer to avoid unaligned
access.

Fixes: 85709cbf1524 ("media: replace strncpy() by strscpy()")
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
Change in v3:
- fix sparse warnings.
Change in v2:
- fix sparse warnings.
---
 drivers/media/platform/mtk-vpu/mtk_vpu.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c b/drivers/media/platform/mtk-vpu/mtk_vpu.c
index a768707abb94..e3fd2d1814f3 100644
--- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
+++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
@@ -603,12 +603,14 @@ EXPORT_SYMBOL_GPL(vpu_load_firmware);
 static void vpu_init_ipi_handler(void *data, unsigned int len, void *priv)
 {
 	struct mtk_vpu *vpu = (struct mtk_vpu *)priv;
-	struct vpu_run *run = (struct vpu_run *)data;
-
-	vpu->run.signaled = run->signaled;
-	strscpy(vpu->run.fw_ver, run->fw_ver, sizeof(vpu->run.fw_ver));
-	vpu->run.dec_capability = run->dec_capability;
-	vpu->run.enc_capability = run->enc_capability;
+	struct vpu_run __iomem *run = (struct vpu_run __iomem __force *)data;
+
+	vpu->run.signaled = readl(&run->signaled);
+	memcpy_fromio(vpu->run.fw_ver, run->fw_ver, sizeof(vpu->run.fw_ver));
+	/* Make sure the string is NUL-terminated */
+	vpu->run.fw_ver[sizeof(vpu->run.fw_ver) - 1] = '\0';
+	vpu->run.dec_capability = readl(&run->dec_capability);
+	vpu->run.enc_capability = readl(&run->enc_capability);
 	wake_up_interruptible(&vpu->run.wq);
 }
 
-- 
2.25.0.265.gbab2e86ba0-goog

Re: [PATCH v3] media: mtk-vpu: avoid unaligned access to DTCM buffer.

[Hans Verkuil]

On 2/25/20 6:24 PM, Hsin-Yi Wang wrote:
> struct vpu_run *run in vpu_init_ipi_handler() is an ioremapped DTCM (Data
> Tightly Coupled Memory) buffer shared with AP.  It's not able to do
> unaligned access. Otherwise kernel would crash due to unable to handle
> kernel paging request.
> 
> struct vpu_run {
> 	u32 signaled;
> 	char fw_ver[VPU_FW_VER_LEN];
> 	unsigned int	dec_capability;
> 	unsigned int	enc_capability;
> 	wait_queue_head_t wq;
> };
> 
> fw_ver starts at 4 byte boundary. If system enables
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, strscpy() will do
> read_word_at_a_time(), which tries to read 8-byte: *(unsigned long *)addr
> 
> Copy the string by memcpy_fromio() for this buffer to avoid unaligned
> access.
> 
> Fixes: 85709cbf1524 ("media: replace strncpy() by strscpy()")
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> Change in v3:
> - fix sparse warnings.
> Change in v2:
> - fix sparse warnings.
> ---
>  drivers/media/platform/mtk-vpu/mtk_vpu.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> index a768707abb94..e3fd2d1814f3 100644
> --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> @@ -603,12 +603,14 @@ EXPORT_SYMBOL_GPL(vpu_load_firmware);
>  static void vpu_init_ipi_handler(void *data, unsigned int len, void *priv)
>  {
>  	struct mtk_vpu *vpu = (struct mtk_vpu *)priv;
> -	struct vpu_run *run = (struct vpu_run *)data;
> -
> -	vpu->run.signaled = run->signaled;
> -	strscpy(vpu->run.fw_ver, run->fw_ver, sizeof(vpu->run.fw_ver));
> -	vpu->run.dec_capability = run->dec_capability;
> -	vpu->run.enc_capability = run->enc_capability;
> +	struct vpu_run __iomem *run = (struct vpu_run __iomem __force *)data;

The use of __force is generally a bad sign. Shouldn't the 'void *data' be a
'void __iomem *data'? And vpu->recv_buf should be 'struct share_obj __iomem *recv_buf;'.
Probably send_buf as well.

In other words, the __iomem attribute should be wired up correctly throughout the
driver code, and not forcibly applied in one place. That is asking for trouble in
the future. Also, sparse only works well in detecting problems if such attributes
are applied at the right level.

Regards,

	Hans

> +
> +	vpu->run.signaled = readl(&run->signaled);
> +	memcpy_fromio(vpu->run.fw_ver, run->fw_ver, sizeof(vpu->run.fw_ver));
> +	/* Make sure the string is NUL-terminated */
> +	vpu->run.fw_ver[sizeof(vpu->run.fw_ver) - 1] = '\0';
> +	vpu->run.dec_capability = readl(&run->dec_capability);
> +	vpu->run.enc_capability = readl(&run->enc_capability);
>  	wake_up_interruptible(&vpu->run.wq);
>  }
>  
> 

Re: [PATCH v3] media: mtk-vpu: avoid unaligned access to DTCM buffer.

[Hsin-Yi Wang]

On Thu, Feb 27, 2020 at 5:50 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 2/25/20 6:24 PM, Hsin-Yi Wang wrote:
> > struct vpu_run *run in vpu_init_ipi_handler() is an ioremapped DTCM (Data
> > Tightly Coupled Memory) buffer shared with AP.  It's not able to do
> > unaligned access. Otherwise kernel would crash due to unable to handle
> > kernel paging request.
> >
> > struct vpu_run {
> >       u32 signaled;
> >       char fw_ver[VPU_FW_VER_LEN];
> >       unsigned int    dec_capability;
> >       unsigned int    enc_capability;
> >       wait_queue_head_t wq;
> > };
> >
> > fw_ver starts at 4 byte boundary. If system enables
> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, strscpy() will do
> > read_word_at_a_time(), which tries to read 8-byte: *(unsigned long *)addr
> >
> > Copy the string by memcpy_fromio() for this buffer to avoid unaligned
> > access.
> >
> > Fixes: 85709cbf1524 ("media: replace strncpy() by strscpy()")
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > Change in v3:
> > - fix sparse warnings.
> > Change in v2:
> > - fix sparse warnings.
> > ---
> >  drivers/media/platform/mtk-vpu/mtk_vpu.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> > index a768707abb94..e3fd2d1814f3 100644
> > --- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
> > +++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
> > @@ -603,12 +603,14 @@ EXPORT_SYMBOL_GPL(vpu_load_firmware);
> >  static void vpu_init_ipi_handler(void *data, unsigned int len, void *priv)
> >  {
> >       struct mtk_vpu *vpu = (struct mtk_vpu *)priv;
> > -     struct vpu_run *run = (struct vpu_run *)data;
> > -
> > -     vpu->run.signaled = run->signaled;
> > -     strscpy(vpu->run.fw_ver, run->fw_ver, sizeof(vpu->run.fw_ver));
> > -     vpu->run.dec_capability = run->dec_capability;
> > -     vpu->run.enc_capability = run->enc_capability;
> > +     struct vpu_run __iomem *run = (struct vpu_run __iomem __force *)data;
>
> The use of __force is generally a bad sign. Shouldn't the 'void *data' be a
> 'void __iomem *data'? And vpu->recv_buf should be 'struct share_obj __iomem *recv_buf;'.
> Probably send_buf as well.
>
> In other words, the __iomem attribute should be wired up correctly throughout the
> driver code, and not forcibly applied in one place. That is asking for trouble in
> the future. Also, sparse only works well in detecting problems if such attributes
> are applied at the right level.
>
> Regards,
>
>         Hans
>
Thanks for your comments. I should check the whole code more
thoroughly. I do see that vpu->recv_buf is forced cast from void
__iomem *tcm:
vpu->recv_buf = (__force struct share_obj *)(vpu->reg.tcm +VPU_DTCM_OFFSET);
I'll use struct share_obj __iomem *recv_buf; as you suggested. Thanks

Since all handlers (vpu_init_ipi_handler, vpu_enc_ipi_handler,
vpu_dec_ipi_handler, and mtk_mdp_vpu_ipi_handler) only do read access
to this buffer, I think we can also change 'void *data' as 'const void
*data', and pass another buffer copied from vpu->recv_buf->share_buf
to handler. In this way we don't have to change to use iomem APIs in
those handlers.

 static void vpu_ipi_handler(struct mtk_vpu *vpu)
 {
-       struct share_obj *rcv_obj = vpu->recv_buf;
+       struct share_obj __iomem *rcv_obj = vpu->recv_buf;
        struct vpu_ipi_desc *ipi_desc = vpu->ipi_desc;
-
-       if (rcv_obj->id < IPI_MAX && ipi_desc[rcv_obj->id].handler) {
-               ipi_desc[rcv_obj->id].handler(rcv_obj->share_buf,...
...
+       unsigned char data[SHARE_BUF_SIZE];
+       s32 id = readl(&rcv_obj->id);
+
+       memcpy_fromio(data, rcv_obj->share_buf, sizeof(data));
+       if (id < IPI_MAX && ipi_desc[id].handler) {
+               ipi_desc[id].handler(data, readl(&rcv_obj->len), ...
...