Tegra DRM fixes

Commit bdd2f9cd ("Don't leak kernel pointer to userspace") added a mutex around staging IOCTL's, some of those mutexes are taken twice.

Fixes bdd2f9cd10eb ("drm/tegra: Don't leak kernel pointer to userspace")

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Mikko Perttunen <[email protected]>

Commit 33a8eb8 ("Implement runtime PM") introduced HW reset control. It causes a hang on Tegra20 if both display controllers are utilized (RGB panel and HDMI). The TRM suggests that each display controller has its own reset control, apparently it is not correct.

Fixes 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")

Signed-off-by Dmitry Osipenko <[email protected]>

On Tegra20 an overlay plane should be clipped, otherwise its output is distorted once plane crosses display boundary.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/gpu/drm/tegra/dc.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

drivers/gpu/drm/tegra/dc.c +14 -8

index 98ee6abb056c..a7a7cce1afd0 100644
@@ -486,12 +486,25 @@ static int tegra_plane_state_add(struct tegra_plane *plane,
 {
        struct drm_crtc_state *crtc_state;
        struct tegra_dc_state *tegra;
+       struct drm_rect clip;
+       int err;
 
        /* Propagate errors from allocation or locking failures. */
        crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
        if (IS_ERR(crtc_state))
                return PTR_ERR(crtc_state);
 
+       clip.x1 = 0;
+       clip.y1 = 0;
+       clip.x2 = crtc_state->mode.hdisplay;
+       clip.y2 = crtc_state->mode.vdisplay;
+
+       /* Check plane state for visibility and calculate clipping bounds */
+       err = drm_plane_helper_check_state(state, &clip, 0, INT_MAX,
+                                          true, true);
+       if (err < 0)
+               return err;
+
        tegra = to_dc_state(crtc_state);
 
        tegra->planes |= WIN_A_ACT_REQ << plane->index;

@@ -561,14 +574,14 @@ static void tegra_plane_atomic_update(struct drm_plane *plane,
                return;
 
        memset(&window, 0, sizeof(window));
-       window.src.x = plane->state->src_x >> 16;
-       window.src.y = plane->state->src_y >> 16;
-       window.src.w = plane->state->src_w >> 16;
-       window.src.h = plane->state->src_h >> 16;
-       window.dst.x = plane->state->crtc_x;
-       window.dst.y = plane->state->crtc_y;
-       window.dst.w = plane->state->crtc_w;
-       window.dst.h = plane->state->crtc_h;
+       window.src.x = plane->state->src.x1 >> 16;

On Tegra20 if plane has width or height equal to 0, it will be infinitely wide or tall. Let's disable the plane if it is invisible on atomic state committing to fix the issue. The Rockchip DRM driver does the same.

Signed-off-by Dmitry Osipenko <[email protected]>

There is no IOMMU on Tegra20, instead a GART would be picked as an IOMMU provider.

Signed-off-by Dmitry Osipenko <[email protected]>

Acked-by Joerg Roedel <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c | 3 ++-
 drivers/gpu/host1x/dev.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/tegra/drm.c +2 -1

index e999391aedc9..aa7988dcc28f 100644
@@ -131,7 +131,8 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
        if (!tegra)
                return -ENOMEM;
 
-       if (iommu_present(&platform_bus_type)) {
+       if (iommu_present(&platform_bus_type) &&
+           !of_machine_is_compatible("nvidia,tegra20")) {
                u64 carveout_start, carveout_end, gem_start, gem_end;
                struct iommu_domain_geometry *geometry;
                unsigned long order;

drivers/gpu/host1x/dev.c +0 -0

index f05ebb14fa63..6a805ed908c3 100644

Quoted 29 lines · Dmitry Osipenko

There is no IOMMU on Tegra20, instead a GART would be picked as an IOMMU provider.

Signed-off-by Dmitry Osipenko <[email protected]>

Acked-by Joerg Roedel <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c | 3 ++-
 drivers/gpu/host1x/dev.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/tegra/drm.c +2 -1

index e999391aedc9..aa7988dcc28f 100644
@@ -131,7 +131,8 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
        if (!tegra)
                return -ENOMEM;
 
-       if (iommu_present(&platform_bus_type)) {
+       if (iommu_present(&platform_bus_type) &&
+           !of_machine_is_compatible("nvidia,tegra20")) {
                u64 carveout_start, carveout_end, gem_start, gem_end;
                struct iommu_domain_geometry *geometry;
                unsigned long order;

drivers/gpu/host1x/dev.c +0 -0

index f05ebb14fa63..6a805ed908c3 100644

drivers/gpu/host1x/dev.c static int host1x_probe(struct platform_device *pdev) · line 184

Quoted 9 lines

drivers/gpu/host1x/dev.c +2 -1

@@ -177,7 +177,8 @@ static int host1x_probe(struct platform_device *pdev)
                return err;
        }
 
-       if (iommu_present(&platform_bus_type)) {
+       if (iommu_present(&platform_bus_type) &&
+           !of_machine_is_compatible("nvidia,tegra20")) {
                struct iommu_domain_geometry *geometry;
                unsigned long order;
 

This doesn't feel great... The commit message says there's no IOMMU, but iommu_present says otherwise. I know there's some more subtleties here, and the commit message does hint at this. But...

If we don't want to treat the GART as an IOMMU, shouldn't we somehow make sure iommu_present() doesn't return true in these cases (or perhaps make something like tegra_use_iommu(), with a comment explaining why we don't want to allow the GART to be treated like a proper IOMMU)? These seems to be the only Tegra-specific calls to iommu_present()...

That being said, the patch seems to have the right effect...

Quoted 44 lines · Erik Faye-Lund

Quoted 29 lines · Dmitry Osipenko

There is no IOMMU on Tegra20, instead a GART would be picked as an IOMMU provider.

Signed-off-by Dmitry Osipenko <[email protected]>

Acked-by Joerg Roedel <[email protected]>

---
 drivers/gpu/drm/tegra/drm.c | 3 ++-
 drivers/gpu/host1x/dev.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/gpu/drm/tegra/drm.c +2 -1

index e999391aedc9..aa7988dcc28f 100644
@@ -131,7 +131,8 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
        if (!tegra)
                return -ENOMEM;
 
-       if (iommu_present(&platform_bus_type)) {
+       if (iommu_present(&platform_bus_type) &&
+           !of_machine_is_compatible("nvidia,tegra20")) {
                u64 carveout_start, carveout_end, gem_start, gem_end;
                struct iommu_domain_geometry *geometry;
                unsigned long order;

drivers/gpu/host1x/dev.c +0 -0

index f05ebb14fa63..6a805ed908c3 100644

drivers/gpu/host1x/dev.c static int host1x_probe(struct platform_device *pdev) · line 184

Quoted 9 lines

drivers/gpu/host1x/dev.c +2 -1

@@ -177,7 +177,8 @@ static int host1x_probe(struct platform_device *pdev)
                return err;
        }
 
-       if (iommu_present(&platform_bus_type)) {
+       if (iommu_present(&platform_bus_type) &&
+           !of_machine_is_compatible("nvidia,tegra20")) {
                struct iommu_domain_geometry *geometry;
                unsigned long order;
 

This doesn't feel great... The commit message says there's no IOMMU, but iommu_present says otherwise. I know there's some more subtleties here, and the commit message does hint at this. But...

Quoted 9 lines

If we don't want to treat the GART as an IOMMU, shouldn't we somehow make sure iommu_present() doesn't return true in these cases (or perhaps make something like tegra_use_iommu(), with a comment explaining why we don't want to allow the GART to be treated like a proper IOMMU)? These seems to be the only Tegra-specific calls to iommu_present()...

That being said, the patch seems to have the right effect...

We don't want to treat the GART as an IOMMU right now, but I'd want to change that in the (near?) future, so it's a kinda trivial preparation patch that restores the GART driver and these of_machine_is_compatible() are supposed to go away later. Probably I should mention this in the commit message(?).

I think we can add a Tegra20 specific IOCTL for an allocation of GART-able memory and use it for a stuff like opentegra's EXA, its fallback allocations migration is a pita and reserved CMA is never enough =)

The commands stream is prepended by the jobs class on the CDMA submission, so that explicitly setting a module class in the commands stream isn't necessary. The firewall initializes its class to 0 and the command stream that doesn't explicitly specify the class effectively bypasses the firewall.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Mikko Perttunen <[email protected]>

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
 drivers/gpu/host1x/dev.c |  4 ++++
 drivers/gpu/host1x/dev.h |  1 +
 drivers/gpu/host1x/job.c | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

drivers/gpu/host1x/dev.c +4 -0

index 6a805ed908c3..21da331ce092 100644
@@ -72,6 +72,7 @@ static const struct host1x_info host1x01_info = {
 	.init = host1x01_init,
 	.sync_offset = 0x3000,
 	.dma_mask = DMA_BIT_MASK(32),
+	.version = 1,
 };
 
 static const struct host1x_info host1x02_info = {

@@ -82,6 +83,7 @@ static const struct host1x_info host1x02_info = {
 	.init = host1x02_init,
 	.sync_offset = 0x3000,
 	.dma_mask = DMA_BIT_MASK(32),
+	.version = 2,
 };
 
 static const struct host1x_info host1x04_info = {

@@ -92,6 +94,7 @@ static const struct host1x_info host1x04_info = {
 	.init = host1x04_init,
 	.sync_offset = 0x2100,
 	.dma_mask = DMA_BIT_MASK(34),
+	.version = 4,
 };
 
 static const struct host1x_info host1x05_info = {

@@ -102,6 +105,7 @@ static const struct host1x_info host1x05_info = {
 	.init = host1x05_init,
 	.sync_offset = 0x2100,
 	.dma_mask = DMA_BIT_MASK(34),
+	.version = 5,
 };
 
 static const struct of_device_id host1x_of_match[] = {

drivers/gpu/host1x/dev.h +1 -0

index 229d08b6a45e..c6997651b336 100644
@@ -100,6 +100,7 @@ struct host1x_info {
 	int (*init)(struct host1x *host1x); /* initialize per SoC ops */
 	unsigned int sync_offset; /* offset of syncpoint registers */
 	u64 dma_mask; /* mask of addressable memory */
+	unsigned int version; /* host1x's version */
 };
 
 struct host1x {

drivers/gpu/host1x/job.c +0 -0

index 4208329ca2af..7825643da324 100644
@@ -333,6 +333,27 @@ static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf,
 	return true;
 }

drivers/gpu/host1x/job.c +10 -0

+	/* relocation shift value validation isn't implemented yet */
+	if (reloc->shift)
+		return false;
+
+	return true;
+}
+
 struct host1x_firewall {
 	struct host1x_job *job;
 	struct device *dev;

@@ -359,6 +380,9 @@ static int check_register(struct host1x_firewall *fw, unsigned long offset)
 		if (!check_reloc(fw->reloc, fw->cmdbuf, fw->offset))
 			return -EINVAL;
 
+		if (!check_reloc_shift(fw->dev, fw->reloc))
+			return -EINVAL;
+
 		fw->num_relocs--;
 		fw->reloc++;
 	}

Quoted 71 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
 drivers/gpu/host1x/dev.c |  4 ++++
 drivers/gpu/host1x/dev.h |  1 +
 drivers/gpu/host1x/job.c | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

drivers/gpu/host1x/dev.c +4 -0

index 6a805ed908c3..21da331ce092 100644
@@ -72,6 +72,7 @@ static const struct host1x_info host1x01_info = {
     .init = host1x01_init,
     .sync_offset = 0x3000,
     .dma_mask = DMA_BIT_MASK(32),
+    .version = 1,
 };
 
 static const struct host1x_info host1x02_info = {

@@ -82,6 +83,7 @@ static const struct host1x_info host1x02_info = {
     .init = host1x02_init,
     .sync_offset = 0x3000,
     .dma_mask = DMA_BIT_MASK(32),
+    .version = 2,
 };
 
 static const struct host1x_info host1x04_info = {

@@ -92,6 +94,7 @@ static const struct host1x_info host1x04_info = {
     .init = host1x04_init,
     .sync_offset = 0x2100,
     .dma_mask = DMA_BIT_MASK(34),
+    .version = 4,
 };
 
 static const struct host1x_info host1x05_info = {

@@ -102,6 +105,7 @@ static const struct host1x_info host1x05_info = {
     .init = host1x05_init,
     .sync_offset = 0x2100,
     .dma_mask = DMA_BIT_MASK(34),
+    .version = 5,
 };
 
 static const struct of_device_id host1x_of_match[] = {

drivers/gpu/host1x/dev.h +1 -0

index 229d08b6a45e..c6997651b336 100644
@@ -100,6 +100,7 @@ struct host1x_info {
     int (*init)(struct host1x *host1x); /* initialize per SoC ops */
     unsigned int sync_offset; /* offset of syncpoint registers */
     u64 dma_mask; /* mask of addressable memory */
+    unsigned int version; /* host1x's version */
 };
 
 struct host1x {

drivers/gpu/host1x/job.c +0 -0

index 4208329ca2af..7825643da324 100644
@@ -333,6 +333,27 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
     return true;
 }
 

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, · line 344

Quoted 9 lines

drivers/gpu/host1x/job.c +9 -0

+static bool check_reloc_shift(struct device *dev, struct host1x_reloc *reloc)
+{
+    struct host1x *host = dev_get_drvdata(dev->parent);
+
+    /* skip newer Tegra's since IOMMU is supposed to be used by
+     * them and not all address registers with their shifts are
+     * publicly documented */
+    if (host->info->version > 1)
+        return true;

I don't like the firewall working differently per SoC - IOMMU can still be disabled on later SoCs and in this case there would be a security hole even if the firewall is enabled.

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, · line 349

Quoted 5 lines

drivers/gpu/host1x/job.c +5 -0

+
+    /* skip Tegra30 with IOMMU enabled */
+    if (host->domain)
+        return true;
+

So if we want to be able to test the firewall on all SoCs just having this check here for all SoCs would be better.

Quoted 22 lines

drivers/gpu/host1x/job.c +7 -0

+    /* relocation shift value validation isn't implemented yet */
+    if (reloc->shift)
+        return false;
+
+    return true;
+}
+
 struct host1x_firewall {
     struct host1x_job *job;

     struct device *dev;

drivers/gpu/host1x/job.c +3 -0

@@ -359,6 +380,9 @@ static int check_register(struct host1x_firewall *fw,
 unsigned long offset)
         if (!check_reloc(fw->reloc, fw->cmdbuf, fw->offset))
             return -EINVAL;
 
+        if (!check_reloc_shift(fw->dev, fw->reloc))
+            return -EINVAL;
+
         fw->num_relocs--;
         fw->reloc++;

     }

Quoted 72 lines · Mikko Perttunen

Quoted 71 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
 drivers/gpu/host1x/dev.c |  4 ++++
 drivers/gpu/host1x/dev.h |  1 +
 drivers/gpu/host1x/job.c | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

drivers/gpu/host1x/dev.c +4 -0

index 6a805ed908c3..21da331ce092 100644
@@ -72,6 +72,7 @@ static const struct host1x_info host1x01_info = {
     .init = host1x01_init,
     .sync_offset = 0x3000,
     .dma_mask = DMA_BIT_MASK(32),
+    .version = 1,
 };
 
 static const struct host1x_info host1x02_info = {

@@ -82,6 +83,7 @@ static const struct host1x_info host1x02_info = {
     .init = host1x02_init,
     .sync_offset = 0x3000,
     .dma_mask = DMA_BIT_MASK(32),
+    .version = 2,
 };
 
 static const struct host1x_info host1x04_info = {

@@ -92,6 +94,7 @@ static const struct host1x_info host1x04_info = {
     .init = host1x04_init,
     .sync_offset = 0x2100,
     .dma_mask = DMA_BIT_MASK(34),
+    .version = 4,
 };
 
 static const struct host1x_info host1x05_info = {

@@ -102,6 +105,7 @@ static const struct host1x_info host1x05_info = {
     .init = host1x05_init,
     .sync_offset = 0x2100,
     .dma_mask = DMA_BIT_MASK(34),
+    .version = 5,
 };
 
 static const struct of_device_id host1x_of_match[] = {

drivers/gpu/host1x/dev.h +1 -0

index 229d08b6a45e..c6997651b336 100644
@@ -100,6 +100,7 @@ struct host1x_info {
     int (*init)(struct host1x *host1x); /* initialize per SoC ops */
     unsigned int sync_offset; /* offset of syncpoint registers */
     u64 dma_mask; /* mask of addressable memory */
+    unsigned int version; /* host1x's version */
 };
 
 struct host1x {

drivers/gpu/host1x/job.c +0 -0

index 4208329ca2af..7825643da324 100644
@@ -333,6 +333,27 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
     return true;
 }
 

Quoted 14 lines

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, · line 344

Quoted 9 lines

drivers/gpu/host1x/job.c +9 -0

+static bool check_reloc_shift(struct device *dev, struct host1x_reloc *reloc)
+{
+    struct host1x *host = dev_get_drvdata(dev->parent);
+
+    /* skip newer Tegra's since IOMMU is supposed to be used by
+     * them and not all address registers with their shifts are
+     * publicly documented */
+    if (host->info->version > 1)
+        return true;

I don't like the firewall working differently per SoC - IOMMU can still be disabled on later SoCs and in this case there would be a security hole even if the firewall is enabled.

Yes, it would be a security hole like it is is right now. Unfortunately we can't fix it on later SoC's without knowing all the address registers and their shifts.

Quoted 10 lines

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, struct host1x_bo *cmdbuf, · line 349

Quoted 6 lines

drivers/gpu/host1x/job.c +5 -0

+
+    /* skip Tegra30 with IOMMU enabled */
+    if (host->domain)
+        return true;
+

So if we want to be able to test the firewall on all SoCs just having this check here for all SoCs would be better.

I'm not sure what you are meaning here. You'll get a wrong address resolved in HW if that address needs to be shifted.

As I wrote before, on later SoC's firewall could be used as a debug feature without the shifts being validated.

Quoted 22 lines

Quoted 22 lines

drivers/gpu/host1x/job.c +7 -0

+    /* relocation shift value validation isn't implemented yet */
+    if (reloc->shift)
+        return false;
+
+    return true;
+}
+
 struct host1x_firewall {
     struct host1x_job *job;

     struct device *dev;

drivers/gpu/host1x/job.c +3 -0

@@ -359,6 +380,9 @@ static int check_register(struct host1x_firewall *fw,
 unsigned long offset)
         if (!check_reloc(fw->reloc, fw->cmdbuf, fw->offset))
             return -EINVAL;
 
+        if (!check_reloc_shift(fw->dev, fw->reloc))
+            return -EINVAL;
+
         fw->num_relocs--;
         fw->reloc++;

     }

Quoted 88 lines · Dmitry Osipenko

Quoted 72 lines · Mikko Perttunen

Quoted 71 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
 drivers/gpu/host1x/dev.c |  4 ++++
 drivers/gpu/host1x/dev.h |  1 +
 drivers/gpu/host1x/job.c | 24 ++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

drivers/gpu/host1x/dev.c +4 -0

index 6a805ed908c3..21da331ce092 100644
@@ -72,6 +72,7 @@ static const struct host1x_info host1x01_info = {
     .init = host1x01_init,
     .sync_offset = 0x3000,
     .dma_mask = DMA_BIT_MASK(32),
+    .version = 1,
 };
 
 static const struct host1x_info host1x02_info = {

@@ -82,6 +83,7 @@ static const struct host1x_info host1x02_info = {
     .init = host1x02_init,
     .sync_offset = 0x3000,
     .dma_mask = DMA_BIT_MASK(32),
+    .version = 2,
 };
 
 static const struct host1x_info host1x04_info = {

@@ -92,6 +94,7 @@ static const struct host1x_info host1x04_info = {
     .init = host1x04_init,
     .sync_offset = 0x2100,
     .dma_mask = DMA_BIT_MASK(34),
+    .version = 4,
 };
 
 static const struct host1x_info host1x05_info = {

@@ -102,6 +105,7 @@ static const struct host1x_info host1x05_info = {
     .init = host1x05_init,
     .sync_offset = 0x2100,
     .dma_mask = DMA_BIT_MASK(34),
+    .version = 5,
 };
 
 static const struct of_device_id host1x_of_match[] = {

drivers/gpu/host1x/dev.h +1 -0

index 229d08b6a45e..c6997651b336 100644
@@ -100,6 +100,7 @@ struct host1x_info {
     int (*init)(struct host1x *host1x); /* initialize per SoC ops */
     unsigned int sync_offset; /* offset of syncpoint registers */
     u64 dma_mask; /* mask of addressable memory */
+    unsigned int version; /* host1x's version */
 };
 
 struct host1x {

drivers/gpu/host1x/job.c +0 -0

index 4208329ca2af..7825643da324 100644
@@ -333,6 +333,27 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
     return true;
 }
 

Quoted 14 lines

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, · line 345

Quoted 9 lines

drivers/gpu/host1x/job.c +9 -0

+static bool check_reloc_shift(struct device *dev, struct host1x_reloc *reloc)
+{
+    struct host1x *host = dev_get_drvdata(dev->parent);
+
+    /* skip newer Tegra's since IOMMU is supposed to be used by
+     * them and not all address registers with their shifts are
+     * publicly documented */
+    if (host->info->version > 1)
+        return true;

I don't like the firewall working differently per SoC - IOMMU can still be disabled on later SoCs and in this case there would be a security hole even if the firewall is enabled.

Quoted 2 lines

Yes, it would be a security hole like it is is right now. Unfortunately we can't fix it on later SoC's without knowing all the address registers and their shifts.

Indeed, we can't fix it, but I was thinking that having the firewall enabled in the kernel should always mean that the system is safe, even if IOMMU is disabled, and even if it means that you cannot really do anything useful with it (at least running host1x_test should be possible without shifts, though). Thus no matter what the hardware revision, we should reject anything we cannot ensure as safe.

Quoted 11 lines

Quoted 10 lines

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, · line 350

Quoted 6 lines

drivers/gpu/host1x/job.c +5 -0

+
+    /* skip Tegra30 with IOMMU enabled */
+    if (host->domain)
+        return true;
+

So if we want to be able to test the firewall on all SoCs just having this check here for all SoCs would be better.

Quoted 2 lines

I'm not sure what you are meaning here. You'll get a wrong address resolved in HW if that address needs to be shifted.

So we have two choices here:
a) If IOMMU is enabled, skip the shift check and return true always. This means that we still ensure that the system is safe, since with IOMMU enabled it is safe even if the firewall cannot ensure that the job is proper. This is what I was trying to say above.
b) Always return false if there is a shift, on any SoC and even if IOMMU is enabled. This would harmonize the behavior of the firewall across systems.

Quoted 115 lines · Mikko Perttunen

Quoted 88 lines · Dmitry Osipenko

Quoted 72 lines · Mikko Perttunen

Quoted 71 lines · Dmitry Osipenko

Incorrectly shifted relocation address will cause a lower memory corruption and likely a hang on a write or a read of an arbitrary data in case of IOMMU absent. As of now there is no use for the address shifting (at least on Tegra20) and adding a proper shifting / sizes validation is much more work. Let's forbid it in the firewall till a proper validation is implemented.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Erik Faye-Lund <[email protected]>

---
  drivers/gpu/host1x/dev.c |  4 ++++
  drivers/gpu/host1x/dev.h |  1 +
  drivers/gpu/host1x/job.c | 24 ++++++++++++++++++++++++
  3 files changed, 29 insertions(+)

drivers/gpu/host1x/dev.c +4 -0

index 6a805ed908c3..21da331ce092 100644
@@ -72,6 +72,7 @@ static const struct host1x_info host1x01_info = {
      .init = host1x01_init,
      .sync_offset = 0x3000,
      .dma_mask = DMA_BIT_MASK(32),
+    .version = 1,
  };
 
  static const struct host1x_info host1x02_info = {

@@ -82,6 +83,7 @@ static const struct host1x_info host1x02_info = {
      .init = host1x02_init,
      .sync_offset = 0x3000,
      .dma_mask = DMA_BIT_MASK(32),
+    .version = 2,
  };
 
  static const struct host1x_info host1x04_info = {

@@ -92,6 +94,7 @@ static const struct host1x_info host1x04_info = {
      .init = host1x04_init,
      .sync_offset = 0x2100,
      .dma_mask = DMA_BIT_MASK(34),
+    .version = 4,
  };
 
  static const struct host1x_info host1x05_info = {

@@ -102,6 +105,7 @@ static const struct host1x_info host1x05_info = {
      .init = host1x05_init,
      .sync_offset = 0x2100,
      .dma_mask = DMA_BIT_MASK(34),
+    .version = 5,
  };
 
  static const struct of_device_id host1x_of_match[] = {

drivers/gpu/host1x/dev.h +1 -0

index 229d08b6a45e..c6997651b336 100644
@@ -100,6 +100,7 @@ struct host1x_info {
      int (*init)(struct host1x *host1x); /* initialize per SoC ops */
      unsigned int sync_offset; /* offset of syncpoint registers */
      u64 dma_mask; /* mask of addressable memory */
+    unsigned int version; /* host1x's version */
  };
 
  struct host1x {

drivers/gpu/host1x/job.c +0 -0

index 4208329ca2af..7825643da324 100644
@@ -333,6 +333,27 @@ static bool check_reloc(struct host1x_reloc *reloc,
 struct host1x_bo *cmdbuf,
      return true;
  }
 

Quoted 14 lines

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, · line 345

Quoted 9 lines

drivers/gpu/host1x/job.c +9 -0

+static bool check_reloc_shift(struct device *dev, struct host1x_reloc *reloc)
+{
+    struct host1x *host = dev_get_drvdata(dev->parent);
+
+    /* skip newer Tegra's since IOMMU is supposed to be used by
+     * them and not all address registers with their shifts are
+     * publicly documented */
+    if (host->info->version > 1)
+        return true;

I don't like the firewall working differently per SoC - IOMMU can still be disabled on later SoCs and in this case there would be a security hole even if the firewall is enabled.

Quoted 2 lines

Yes, it would be a security hole like it is is right now. Unfortunately we can't fix it on later SoC's without knowing all the address registers and their shifts.

Indeed, we can't fix it, but I was thinking that having the firewall enabled in the kernel should always mean that the system is safe, even if IOMMU is disabled, and even if it means that you cannot really do anything useful with it (at least running host1x_test should be possible without shifts, though). Thus no matter what the hardware revision, we should reject anything we cannot ensure as safe.

Quoted 11 lines

Quoted 10 lines

drivers/gpu/host1x/job.c static bool check_reloc(struct host1x_reloc *reloc, · line 350

Quoted 6 lines

drivers/gpu/host1x/job.c +5 -0

+
+    /* skip Tegra30 with IOMMU enabled */
+    if (host->domain)
+        return true;
+

So if we want to be able to test the firewall on all SoCs just having this check here for all SoCs would be better.

Quoted 2 lines

I'm not sure what you are meaning here. You'll get a wrong address resolved in HW if that address needs to be shifted.

Quoted 7 lines

So we have two choices here:
a) If IOMMU is enabled, skip the shift check and return true always. This means that we still ensure that the system is safe, since with IOMMU enabled it is safe even if the firewall cannot ensure that the job is proper. This is what I was trying to say above.
b) Always return false if there is a shift, on any SoC and even if IOMMU is enabled. This would harmonize the behavior of the firewall across systems.

Okay, I think b) is a better choice, actually that's what V1 of the patch did. Preserving behaviour regardless of the IOMMU state is reasonable and this is an opensource driver that is supposed to be used with the opensource userspace after all.

Later I'd want to stricter the firewall checks to disallow any unknown register accesses, that will make it a bit more secure on a later SoC's as well. After, we should add a memory access bounds checking for each of HW operations to make it really secure.

Check waits in the firewall in a way it is done for relocations.

Signed-off-by Dmitry Osipenko <[email protected]>

Reviewed-by Mikko Perttunen <[email protected]>

The blocking gather copy allocation is a major performance downside of the Host1x firewall, it may take hundreds milliseconds which is unacceptable for the real-time graphics operations. Let's try a non-blocking allocation first as a least invasive solution, it makes opentegra (Xorg driver) performance indistinguishable with/without the firewall.

Signed-off-by Dmitry Osipenko <[email protected]>

---

The other much more invasive solution could be: to have a memory pool reserved for the gather copy and performing the firewall checks (and the gather copying) on the actual job submission to the CDMA, not on the job allocation like it is done now. This solution reduces the overhead of a gather copy allocations.

 drivers/gpu/host1x/job.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/gpu/host1x/job.c +6 -3

index fc194c676d91..ed0575f14093 100644
@@ -594,12 +594,20 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
                size += g->words * sizeof(u32);
        }
 
+       /*
+        * Try a non-blocking allocation from a higher priority pools first,
+        * as awaiting for the allocation here is a major performance hit.
+        */
        job->gather_copy_mapped = dma_alloc_wc(dev, size, &job->gather_copy,
-                                              GFP_KERNEL);
-       if (!job->gather_copy_mapped) {
-               job->gather_copy_mapped = NULL;
+                                              GFP_NOWAIT);
+

Quoted 45 lines · Dmitry Osipenko

The blocking gather copy allocation is a major performance downside of the Host1x firewall, it may take hundreds milliseconds which is unacceptable for the real-time graphics operations. Let's try a non-blocking allocation first as a least invasive solution, it makes opentegra (Xorg driver) performance indistinguishable with/without the firewall.

Signed-off-by Dmitry Osipenko <[email protected]>

---

The other much more invasive solution could be: to have a memory pool reserved for the gather copy and performing the firewall checks (and the gather copying) on the actual job submission to the CDMA, not on the job allocation like it is done now. This solution reduces the overhead of a gather copy allocations.

 drivers/gpu/host1x/job.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/gpu/host1x/job.c +12 -4

index fc194c676d91..ed0575f14093 100644
@@ -594,12 +594,20 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
                size += g->words * sizeof(u32);
        }
 
+       /*
+        * Try a non-blocking allocation from a higher priority pools first,
+        * as awaiting for the allocation here is a major performance hit.
+        */
        job->gather_copy_mapped = dma_alloc_wc(dev, size, &job->gather_copy,
-                                              GFP_KERNEL);
-       if (!job->gather_copy_mapped) {
-               job->gather_copy_mapped = NULL;
+                                              GFP_NOWAIT);
+
+       /* the higher priority allocation failed, try the generic-blocking */
+       if (!job->gather_copy_mapped)
+               job->gather_copy_mapped = dma_alloc_wc(dev, size,
+                                                      &job->gather_copy,
+                                                      GFP_KERNEL);
+       if (!job->gather_copy_mapped)
                return -ENOMEM;
-       }
 

drivers/gpu/host1x/job.c static inline int copy_gathers(struct host1x_job *job, struct device *dev) · line 612

Quoted 1 line

drivers/gpu/host1x/job.c +0 -0

        job->gather_copy_size = size;

Can't we just have a static fixed-size buffer, and validate chunks at the time? Or why can't we validate directly on the mapped pointers?

It feels pretty silly to have to allocate memory in the first place here...

Quoted 51 lines · Erik Faye-Lund

Quoted 36 lines · Dmitry Osipenko

The blocking gather copy allocation is a major performance downside of the Host1x firewall, it may take hundreds milliseconds which is unacceptable for the real-time graphics operations. Let's try a non-blocking allocation first as a least invasive solution, it makes opentegra (Xorg driver) performance indistinguishable with/without the firewall.

Signed-off-by Dmitry Osipenko <[email protected]>

---

The other much more invasive solution could be: to have a memory pool reserved for the gather copy and performing the firewall checks (and the gather copying) on the actual job submission to the CDMA, not on the job allocation like it is done now. This solution reduces the overhead of a gather copy allocations.

 drivers/gpu/host1x/job.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/gpu/host1x/job.c +6 -3

index fc194c676d91..ed0575f14093 100644
@@ -594,12 +594,20 @@ static inline int copy_gathers(struct host1x_job *job, struct device *dev)
                size += g->words * sizeof(u32);
        }
 
+       /*
+        * Try a non-blocking allocation from a higher priority pools first,
+        * as awaiting for the allocation here is a major performance hit.
+        */
        job->gather_copy_mapped = dma_alloc_wc(dev, size, &job->gather_copy,
-                                              GFP_KERNEL);
-       if (!job->gather_copy_mapped) {
-               job->gather_copy_mapped = NULL;
+                                              GFP_NOWAIT);
+

drivers/gpu/host1x/job.c static inline int copy_gathers(struct host1x_job *job, struct device *dev) · line 612

Quoted 10 lines

drivers/gpu/host1x/job.c +6 -1

+       /* the higher priority allocation failed, try the generic-blocking */
+       if (!job->gather_copy_mapped)
+               job->gather_copy_mapped = dma_alloc_wc(dev, size,
+                                                      &job->gather_copy,
+                                                      GFP_KERNEL);
+       if (!job->gather_copy_mapped)
                return -ENOMEM;
-       }
 
        job->gather_copy_size = size;

Can't we just have a static fixed-size buffer, and validate chunks at the time? Or why can't we validate directly on the mapped pointers?

Quoted 2 lines

It feels pretty silly to have to allocate memory in the first place here...

We can't validate the mapped pointers because userspace may write to the buffers memory at any time. Maybe it should be possible to write-protect cmdbuffers memory for the time of its submission and execution, but I'm not sure whether it's worth the effort. The current-proposed solution is efficient and least invasive.