On 05/14/2017 03:45 PM, Dmitry Osipenko wrote:
On 14.05.2017 15:27, Mikko Perttunen wrote:
On 05/12/2017 10:29 PM, Dmitry Osipenko wrote:
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 <digetx@xxxxxxxxx>
---
v2: Take into account the cmdbuf.offset
drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 732c8d98044f..9ad4ac7c08d1 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -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;
+ }
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.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
