| Message ID | 20170512192903.25491-1-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 12, 2017 at 9:29 PM, Dmitry Osipenko <digetx@gmail.com> 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@gmail.com> Looks good, good catch! Reviewed-by: Erik Faye-Lund <kusmabite@gmail.com>
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@gmail.com> > --- > > 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. Cheers, Mikko > + > + bo = to_tegra_bo(gem); > + host1x_job_add_gather(job, &bo->base, > + cmdbuf.words, cmdbuf.offset); > num_cmdbufs--; > cmdbufs++; > } >
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@gmail.com> >> --- >> >> 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.
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@gmail.com> >>> --- >>> >>> 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
On 14.05.2017 15:56, Mikko Perttunen wrote: > > > 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@gmail.com> >>>> --- >>>> >>>> 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. > 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. > Thanks for contributing :) My pleasure ;)
On 12.05.2017 22:29, 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. > For the record, this patch is superseded by the "drm/tegra: Check offsets of a submitted command buffer and of relocations."
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; + } + + bo = to_tegra_bo(gem); + host1x_job_add_gather(job, &bo->base, + cmdbuf.words, cmdbuf.offset); num_cmdbufs--; cmdbufs++; }
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@gmail.com> --- v2: Take into account the cmdbuf.offset drivers/gpu/drm/tegra/drm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)