[PATCH v2] drm/tegra: Check size of a submitted command buffer

[PATCH v2] drm/tegra: Check size of a submitted command buffer

[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 <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

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;
+		}
+
+		bo = to_tegra_bo(gem);
+		host1x_job_add_gather(job, &bo->base,
+				      cmdbuf.words, cmdbuf.offset);
 		num_cmdbufs--;
 		cmdbufs++;
 	}
-- 
2.12.2

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Erik Faye-Lund]

On Fri, May 12, 2017 at 9:29 PM, Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Looks good, good catch!

Reviewed-by: Erik Faye-Lund <kusmabite-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Mikko Perttunen]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>
> 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++;
>  	}
>

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Dmitry Osipenko]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> 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.

-- 
Dmitry

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Mikko Perttunen]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>
>>> 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

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Dmitry Osipenko]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>
>>>> 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 ;)

-- 
Dmitry

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Dmitry Osipenko]

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."

-- 
Dmitry