linux-kernel - RE: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the virtualization context

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: <AS8PR04MB8946734C740B202DC3CA88F08F849@AS8PR04MB8946.eurprd04.prod.outlook.com>
Date:   Tue, 26 Oct 2021 23:47:18 +0000
From:   Leo Li <leoyang.li@....com>
To:     Ioana Ciornei <ioana.ciornei@....com>,
        Diana Madalina Craciun <diana.craciun@....com>
CC:     Youri Querry <youri.querry_1@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: RE: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
 virtualization context



> -----Original Message-----
> From: Ioana Ciornei <ioana.ciornei@....com>
> Sent: Monday, October 18, 2021 10:11 AM
> To: Leo Li <leoyang.li@....com>
> Cc: Youri Querry <youri.querry_1@....com>; linux-kernel@...r.kernel.org;
> Diana Madalina Craciun <diana.craciun@....com>; Ioana Ciornei
> <ioana.ciornei@....com>
> Subject: [PATCH 4/5] soc: fsl: dpio: fix qbman alignment error in the
> virtualization context
> 
> From: Diana Craciun <diana.craciun@....com>
> 
> When running as a guest, under KVM, the CENA region is mapped as device
> memory, so uncacheable. When the memory is mapped as device memory,
> the unaligned accesses are not allowed.  Memcpy is optimized to transfer 8
> bytes at a time regardless of the start address and might cause alignment
> issues.
> 
> Signed-off-by: Diana Craciun <diana.craciun@....com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>

We get the following feedback from Arnd about this patch.  Could you respond to the comments?

"This patch looks very suspicious to me, I don't think it's generally safe to
use memcpy_toio() on a normal pointer, as the __iomem tokens may
be in a separate address range, even though this currently works
on arm64. Adding the  (__iomem void *) cast without a comment that
explains why it's added seems similarly wrong, and finally the
changeset text does not seem to match what the code does:

According to the text, the pointer is to a virtual address mapped as
"device memory" (i.e. PROT_DEVICE_nGnRE or PROT_DEVICE_nGnRnE),
but the code suggests it's actually write-combining normal
(PROT_NORMAL_NC)."

> ---
>  drivers/soc/fsl/dpio/qbman-portal.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/soc/fsl/dpio/qbman-portal.c
> b/drivers/soc/fsl/dpio/qbman-portal.c
> index 3fd54611ed98..ef9cafd12534 100644
> --- a/drivers/soc/fsl/dpio/qbman-portal.c
> +++ b/drivers/soc/fsl/dpio/qbman-portal.c
> @@ -679,9 +679,9 @@ int qbman_swp_enqueue_multiple_direct(struct
> qbman_swp *s,
>  	for (i = 0; i < num_enqueued; i++) {
>  		p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
>  		/* Skip copying the verb */
> -		memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> -		memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> -		       &fd[i], sizeof(*fd));
> +		memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> +		memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> +			    &fd[i], sizeof(*fd));
>  		eqcr_pi++;
>  	}
> 
> @@ -763,9 +763,9 @@ int
> qbman_swp_enqueue_multiple_mem_back(struct qbman_swp *s,
>  	for (i = 0; i < num_enqueued; i++) {
>  		p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
>  		/* Skip copying the verb */
> -		memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> -		memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> -		       &fd[i], sizeof(*fd));
> +		memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> +		memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> +			    &fd[i], sizeof(*fd));
>  		eqcr_pi++;
>  	}
> 
> @@ -837,9 +837,9 @@ int
> qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
>  		p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
>  		cl = (uint32_t *)(&d[i]);
>  		/* Skip copying the verb */
> -		memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> -		memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> -		       &fd[i], sizeof(*fd));
> +		memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> +		memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> +			    &fd[i], sizeof(*fd));
>  		eqcr_pi++;
>  	}
> 
> @@ -907,9 +907,9 @@ int
> qbman_swp_enqueue_multiple_desc_mem_back(struct qbman_swp *s,
>  		p = (s->addr_cena + QBMAN_CENA_SWP_EQCR(eqcr_pi &
> half_mask));
>  		cl = (uint32_t *)(&d[i]);
>  		/* Skip copying the verb */
> -		memcpy(&p[1], &cl[1], EQ_DESC_SIZE_WITHOUT_FD - 1);
> -		memcpy(&p[EQ_DESC_SIZE_FD_START/sizeof(uint32_t)],
> -		       &fd[i], sizeof(*fd));
> +		memcpy_toio((__iomem void *)&p[1], &cl[1],
> EQ_DESC_SIZE_WITHOUT_FD - 1);
> +		memcpy_toio((__iomem void
> *)&p[EQ_DESC_SIZE_FD_START / sizeof(uint32_t)],
> +			    &fd[i], sizeof(*fd));
>  		eqcr_pi++;
>  	}
> 
> --
> 2.33.1