drm/tegra: Check size of a submitted command buffer
PATCH v2 1/1 drm/tegra: Check size of a submitted command buffer
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
[ 46.829393] Unable to handle kernel paging request at virtual address f09b2000
...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)
---
v2: Take into account the cmdbuf.offset
drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
drivers/gpu/drm/tegra/drm.c +14 -4
@@ -361,20 +361,30 @@ int tegra_drm_submit(struct tegra_drm_context *context,while (num_cmdbufs) {struct drm_tegra_cmdbuf cmdbuf; -struct host1x_bo *bo; +struct drm_gem_object *gem; +struct tegra_bo *bo;if (copy_from_user (&cmdbuf, cmdbufs,sizeof (cmdbuf))) { err = -EFAULT ; goto fail; } - bo =host1x_bo_lookup (file, cmdbuf.handle); -if (!bo) { + gem =drm_gem_object_lookup (file, cmdbuf.handle); +if (!gem) { err = -ENOENT ; goto fail; } -host1x_job_add_gather (job, bo, cmdbuf.words, cmdbuf.offset); +drm_gem_object_unreference_unlocked (gem); + +if (cmdbuf.offset + cmdbuf.words *4 > gem->size) { + err = -EINVAL ; + goto fail; + } + + bo =to_tegra_bo (gem); +host1x_job_add_gather (job, &bo->base, + cmdbuf.words, cmdbuf.offset); num_cmdbufs--; cmdbufs++; }
Signature · 1 line
2.12.2
A message that is not in the archive (sent off-list or not yet received): 20170512192903.25491-1-digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Erik Faye-Lund
Reviewed-byQuoted 14 lines · Dmitry Osipenko
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
[ 46.829393] Unable to handle kernel paging request at virtual address f09b2000...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)
Quoted 1 line
Signed-off-by Dmitry Osipenko <[email protected]>
Looks good, good catch!
Quoted 44 lines · Dmitry Osipenko
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
[ 46.829393] Unable to handle kernel paging request at virtual address f09b2000...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)Signed-off-by Dmitry Osipenko <[email protected]>---
v2: Take into account the cmdbuf.offset
drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)drivers/gpu/drm/tegra/drm.c +4
-3@@ -361,20 +361,30 @@ int tegra_drm_submit(struct tegra_drm_context *context,while (num_cmdbufs) {struct drm_tegra_cmdbuf cmdbuf; -struct host1x_bo *bo; +struct drm_gem_object *gem; +struct tegra_bo *bo;if (copy_from_user (&cmdbuf, cmdbufs,sizeof (cmdbuf))) { err = -EFAULT ; goto fail; } - bo =host1x_bo_lookup (file, cmdbuf.handle); -if (!bo) { + gem =drm_gem_object_lookup (file, cmdbuf.handle); +if (!gem) { err = -ENOENT ;
drivers/gpu/drm/tegra/drm.c int tegra_drm_submit(struct tegra_drm_context *context, · line 383
Quoted 10 lines
drivers/gpu/drm/tegra/drm.c +6
-1goto fail; } -host1x_job_add_gather (job, bo, cmdbuf.words, cmdbuf.offset); +drm_gem_object_unreference_unlocked (gem); + +if (cmdbuf.offset + cmdbuf.words *4 > gem->size) { + err = -EINVAL ; + goto fail; + }
Nasty bug! Well found. Two points: the arithmetic here could overflow, so userspace could supply some large values for offset/words and this check would not catch it. A fix would be to do the arithmetic in 64-bit. Also, looking at the code closer, I can't see any bounds checking for relocs either.. That code (host1x_reloc_copy_from_user) is the other place using host1x_bo_lookup, so we could e.g. change host1x_bo_lookup to take offset and words parameters and verify those at the same time.
Cheers,
Mikko
Quoted 8 lines
drivers/gpu/drm/tegra/drm.c +4
-0+ + bo =to_tegra_bo (gem); +host1x_job_add_gather (job, &bo->base, + cmdbuf.words, cmdbuf.offset); num_cmdbufs--; cmdbufs++; }
A message that is not in the archive (sent off-list or not yet received): 5874d720-c911-b2bc-dc69-be2155505280-/[email protected]
Quoted 48 lines · Mikko Perttunen
Quoted 47 lines · Dmitry Osipenko
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
[ 46.829393] Unable to handle kernel paging request at virtual address f09b2000...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)Signed-off-by Dmitry Osipenko <[email protected]>---
v2: Take into account the cmdbuf.offset
drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)drivers/gpu/drm/tegra/drm.c +4
-3@@ -361,20 +361,30 @@ int tegra_drm_submit(struct tegra_drm_context *context,while (num_cmdbufs) {struct drm_tegra_cmdbuf cmdbuf; -struct host1x_bo *bo; +struct drm_gem_object *gem; +struct tegra_bo *bo;if (copy_from_user (&cmdbuf, cmdbufs,sizeof (cmdbuf))) { err = -EFAULT ; goto fail; } - bo =host1x_bo_lookup (file, cmdbuf.handle); -if (!bo) { + gem =drm_gem_object_lookup (file, cmdbuf.handle); +if (!gem) { err = -ENOENT ; goto fail; }
Quoted 16 lines
drivers/gpu/drm/tegra/drm.c int tegra_drm_submit(struct tegra_drm_context *context, · line 383
Quoted 7 lines
drivers/gpu/drm/tegra/drm.c +6
-1-host1x_job_add_gather (job, bo, cmdbuf.words, cmdbuf.offset); +drm_gem_object_unreference_unlocked (gem); + +if (cmdbuf.offset + cmdbuf.words *4 > gem->size) { + err = -EINVAL ; + goto fail; + }Nasty bug! Well found. Two points: the arithmetic here could overflow, so userspace could supply some large values for offset/words and this check would not catch it. A fix would be to do the arithmetic in 64-bit. Also, looking at the code closer, I can't see any bounds checking for relocs either.. That code (
host1x_reloc_copy_from_user) is the other place usinghost1x_bo_lookup, so we could e.g. changehost1x_bo_lookupto take offset and words parameters and verify those at the same time.
Good point, unfortunately there are a lot of ways to abuse the staging API on the IOMMU-less Tegra20 right now.
Signature · 1 line
Dmitry
A message that is not in the archive (sent off-list or not yet received): 2eca9200-727a-5e6f-efa8-da0a7706c7a8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Quoted 65 lines · Dmitry Osipenko
Quoted 48 lines · Mikko Perttunen
Quoted 47 lines · Dmitry Osipenko
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
[ 46.829393] Unable to handle kernel paging request at virtual address f09b2000...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)Signed-off-by Dmitry Osipenko <[email protected]>---
v2: Take into account the cmdbuf.offset
drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)drivers/gpu/drm/tegra/drm.c +4
-3@@ -361,20 +361,30 @@ int tegra_drm_submit(struct tegra_drm_context *context,while (num_cmdbufs) {struct drm_tegra_cmdbuf cmdbuf; -struct host1x_bo *bo; +struct drm_gem_object *gem; +struct tegra_bo *bo;if (copy_from_user (&cmdbuf, cmdbufs,sizeof (cmdbuf))) { err = -EFAULT ; goto fail; } - bo =host1x_bo_lookup (file, cmdbuf.handle); -if (!bo) { + gem =drm_gem_object_lookup (file, cmdbuf.handle); +if (!gem) { err = -ENOENT ; goto fail; }Quoted 16 lines
drivers/gpu/drm/tegra/drm.c int tegra_drm_submit(struct tegra_drm_context *context, · line 383
Quoted 7 lines
drivers/gpu/drm/tegra/drm.c +6
-1-host1x_job_add_gather (job, bo, cmdbuf.words, cmdbuf.offset); +drm_gem_object_unreference_unlocked (gem); + +if (cmdbuf.offset + cmdbuf.words *4 > gem->size) { + err = -EINVAL ; + goto fail; + }Nasty bug! Well found. Two points: the arithmetic here could overflow, so userspace could supply some large values for offset/words and this check would not catch it. A fix would be to do the arithmetic in 64-bit. Also, looking at the code closer, I can't see any bounds checking for relocs either.. That code (
host1x_reloc_copy_from_user) is the other place usinghost1x_bo_lookup, so we could e.g. changehost1x_bo_lookupto take offset and words parameters and verify those at the same time.
Quoted 3 lines
Good point, unfortunately there are a lot of ways to abuse the staging API on the IOMMU-less Tegra20 right now.
Indeed, though I think the relocation issue would be a problem on IOMMU-enabled systems as well. The Tegra20 and firewall have always been in a bit a bad position, as I'm not sure if the firewall was ever used in production during Tegra20's prime time, and of course it was quickly abandoned in downstream when we got IOMMUs on later chips.
Thanks for contributing :)
Mikko
A message that is not in the archive (sent off-list or not yet received): 3eca952f-7c39-d701-15c1-1e243ba864b4-/[email protected]
Quoted 73 lines · Mikko Perttunen
Quoted 66 lines · Dmitry Osipenko
Quoted 49 lines · Mikko Perttunen
Quoted 48 lines · Dmitry Osipenko
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
[ 46.829393] Unable to handle kernel paging request at virtual address
f09b2000
...
[<c04a3ba4>] (host1x_job_pin) from [<c04dfcd0>] (tegra_drm_submit+0x474/0x510) [<c04dfcd0>] (tegra_drm_submit) from [<c04deea0>] (tegra_submit+0x50/0x6c) [<c04deea0>] (tegra_submit) from [<c04c07c0>] (drm_ioctl+0x1e4/0x3ec) [<c04c07c0>] (drm_ioctl) from [<c02541a0>] (do_vfs_ioctl+0x9c/0x8e4) [<c02541a0>] (do_vfs_ioctl) from [<c0254a1c>] (SyS_ioctl+0x34/0x5c) [<c0254a1c>] (SyS_ioctl) from [<c0107640>] (ret_fast_syscall+0x0/0x3c)Signed-off-by Dmitry Osipenko <[email protected]>---
v2: Take into account the cmdbuf.offset
drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)drivers/gpu/drm/tegra/drm.c +4
-3@@ -361,20 +361,30 @@ int tegra_drm_submit(struct tegra_drm_context *context,while (num_cmdbufs) {struct drm_tegra_cmdbuf cmdbuf; -struct host1x_bo *bo; +struct drm_gem_object *gem; +struct tegra_bo *bo;if (copy_from_user (&cmdbuf, cmdbufs,sizeof (cmdbuf))) { err = -EFAULT ; goto fail; } - bo =host1x_bo_lookup (file, cmdbuf.handle); -if (!bo) { + gem =drm_gem_object_lookup (file, cmdbuf.handle); +if (!gem) { err = -ENOENT ; goto fail; }Quoted 16 lines
drivers/gpu/drm/tegra/drm.c int tegra_drm_submit(struct tegra_drm_context *context, · line 383
Quoted 7 lines
drivers/gpu/drm/tegra/drm.c +6
-1-host1x_job_add_gather (job, bo, cmdbuf.words, cmdbuf.offset); +drm_gem_object_unreference_unlocked (gem); + +if (cmdbuf.offset + cmdbuf.words *4 > gem->size) { + err = -EINVAL ; + goto fail; + }Nasty bug! Well found. Two points: the arithmetic here could overflow, so userspace could supply some large values for offset/words and this check would not catch it. A fix would be to do the arithmetic in 64-bit. Also, looking at the code closer, I can't see any bounds checking for relocs either.. That code (
host1x_reloc_copy_from_user) is the other place usinghost1x_bo_lookup, so we could e.g. changehost1x_bo_lookupto take offset and words parameters and verify those at the same time.Quoted 3 lines
Good point, unfortunately there are a lot of ways to abuse the staging API on the IOMMU-less Tegra20 right now.
Quoted 6 lines
Indeed, though I think the relocation issue would be a problem on IOMMU-enabled systems as well. The Tegra20 and firewall have always been in a bit a bad position, as I'm not sure if the firewall was ever used in production during Tegra20's prime time, and of course it was quickly abandoned in downstream when we got IOMMUs on later chips.
In the IOMMU case some BO will be corrupted in the worst case and it's a userspace problem, while in the non-IOMMU it will be an arbitrary phys memory. Anyway it should be better to fail explicitly in any case.
Quoted 1 line
Thanks for contributing :)
My pleasure ;)
Signature · 1 line
Dmitry
Quoted 5 lines · Dmitry Osipenko
If command buffer claims a number of words that is higher than its BO can fit and a relocation lays past the BO, a kernel OOPS will be fired on that relocation address patching. This was triggered by an opentegra Xorg driver that erroneously pushed too many commands to the pushbuf.
For the record, this patch is superseded by the "drm/tegra: Check offsets of a submitted command buffer and of relocations."
Signature · 1 line
Dmitry