| Message ID | 20220125085634.17972-9-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MT8195 IOMMU SUPPORT | expand |
Il 25/01/22 09:56, Yong Wu ha scritto: > No need zero for the protect buffer that is only accessed by the IOMMU HW > translation fault happened. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> I would rather keep this a devm_kzalloc instead... the cost is very minimal and this will be handy when new hardware will be introduced, as it may require a bigger buffer: in that case, "older" platforms will use only part of it and we may get garbage data at the end. Regards, Angelo
On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno wrote: > Il 25/01/22 09:56, Yong Wu ha scritto: > > No need zero for the protect buffer that is only accessed by the > > IOMMU HW > > translation fault happened. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > I would rather keep this a devm_kzalloc instead... the cost is very > minimal and > this will be handy when new hardware will be introduced, as it may > require a bigger > buffer: in that case, "older" platforms will use only part of it and > we may get > garbage data at the end. Currently this is to avoid zero 512 bytes for all the platforms. Sorry, I don't understand why it is unnecessary when the new hardware requires a bigger buffer. If the buffer becomes bigger, then clearing it to 0 need more cost. then this patch is more helpful? The content in this buffer is garbage, we won't care about or analyse it. > > Regards, > Angelo
On Wed, Feb 16, 2022 at 2:55 PM Yong Wu <yong.wu@mediatek.com> wrote: > > On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno wrote: > > Il 25/01/22 09:56, Yong Wu ha scritto: > > > No need zero for the protect buffer that is only accessed by the > > > IOMMU HW > > > translation fault happened. > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > > I would rather keep this a devm_kzalloc instead... the cost is very > > minimal and > > this will be handy when new hardware will be introduced, as it may > > require a bigger > > buffer: in that case, "older" platforms will use only part of it and > > we may get > > garbage data at the end. > > Currently this is to avoid zero 512 bytes for all the platforms. > > Sorry, I don't understand why it is unnecessary when the new hardware > requires a bigger buffer. If the buffer becomes bigger, then clearing > it to 0 need more cost. then this patch is more helpful? > > The content in this buffer is garbage, we won't care about or analyse > it. I think we should zero it for security reasons regardless of any other aspects. With this patch it's leaking kernel data to the hardware. At the same time, we're talking here about something executed just 1 time when the driver probes. I don't think the cost would really matter. Best regards, Tomasz
On Wed, 2022-02-16 at 14:59 +0900, Tomasz Figa wrote: > On Wed, Feb 16, 2022 at 2:55 PM Yong Wu <yong.wu@mediatek.com> wrote: > > > > On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 25/01/22 09:56, Yong Wu ha scritto: > > > > No need zero for the protect buffer that is only accessed by > > > > the > > > > IOMMU HW > > > > translation fault happened. > > > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > > > > I would rather keep this a devm_kzalloc instead... the cost is > > > very > > > minimal and > > > this will be handy when new hardware will be introduced, as it > > > may > > > require a bigger > > > buffer: in that case, "older" platforms will use only part of it > > > and > > > we may get > > > garbage data at the end. > > > > Currently this is to avoid zero 512 bytes for all the platforms. > > > > Sorry, I don't understand why it is unnecessary when the new > > hardware > > requires a bigger buffer. If the buffer becomes bigger, then > > clearing > > it to 0 need more cost. then this patch is more helpful? > > > > The content in this buffer is garbage, we won't care about or > > analyse > > it. > > I think we should zero it for security reasons regardless of any > other > aspects. With this patch it's leaking kernel data to the hardware. > > At the same time, we're talking here about something executed just 1 > time when the driver probes. I don't think the cost would really > matter. OK. I will remove this patch in next version. Thanks. > > Best regards, > Tomasz > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index e6e4ee471867..d982dfd815c6 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -794,7 +794,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) data->plat_data = of_device_get_match_data(dev); /* Protect memory. HW will access here while translation fault.*/ - protect = devm_kzalloc(dev, MTK_PROTECT_PA_ALIGN * 2, GFP_KERNEL); + protect = devm_kmalloc(dev, MTK_PROTECT_PA_ALIGN * 2, GFP_KERNEL); if (!protect) return -ENOMEM; data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
No need zero for the protect buffer that is only accessed by the IOMMU HW translation fault happened. Signed-off-by: Yong Wu <yong.wu@mediatek.com> --- drivers/iommu/mtk_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)