[PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Joonyoung Shim]

The size of cmdlist is integer type, so it can be overflowed by cmd and
cmd_buf that has too big size. This patch will fix overflow issue as
checking maximum size of cmd and cmd_buf.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index fbd13fa..b31244f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -1250,7 +1250,14 @@ int exynos_g2d_set_cmdlist_ioctl(struct drm_device *drm_dev, void *data,
 		cmdlist->data[cmdlist->last++] = G2D_INTEN_ACF;
 	}
 
-	/* Check size of cmdlist: last 2 is about G2D_BITBLT_START */
+	/* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
+	size = (G2D_CMDLIST_DATA_NUM - cmdlist->last - 2) / 2;
+	if (req->cmd_nr > size || req->cmd_buf_nr > size) {
+		dev_err(dev, "size of cmd or cmd_buf is too big\n");
+		ret = -EINVAL;
+		goto err_free_event;
+	}
+
 	size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
 	if (size > G2D_CMDLIST_DATA_NUM) {
 		dev_err(dev, "cmdlist size is too big\n");
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Tobias Jakobi]

Joonyoung Shim wrote:
> The size of cmdlist is integer type, so it can be overflowed by cmd and
> cmd_buf that has too big size. This patch will fix overflow issue as
> checking maximum size of cmd and cmd_buf.
I don't understand/see the issue here. Could you point out for which
input of the set_cmdlist ioctl you see this particular overflow?

In particular it is not clear to me which size field you're talking
about. struct g2d_cmdlist does not have any field named 'size'.

With best wishes,
Tobias


> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index fbd13fa..b31244f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -1250,7 +1250,14 @@ int exynos_g2d_set_cmdlist_ioctl(struct drm_device *drm_dev, void *data,
>  		cmdlist->data[cmdlist->last++] = G2D_INTEN_ACF;
>  	}
>  
> -	/* Check size of cmdlist: last 2 is about G2D_BITBLT_START */
> +	/* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
> +	size = (G2D_CMDLIST_DATA_NUM - cmdlist->last - 2) / 2;
> +	if (req->cmd_nr > size || req->cmd_buf_nr > size) {
> +		dev_err(dev, "size of cmd or cmd_buf is too big\n");
> +		ret = -EINVAL;
> +		goto err_free_event;
> +	}
> +
>  	size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
>  	if (size > G2D_CMDLIST_DATA_NUM) {
>  		dev_err(dev, "cmdlist size is too big\n");
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Joonyoung Shim]

Hi Tobias,

On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
> Joonyoung Shim wrote:
>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>> cmd_buf that has too big size. This patch will fix overflow issue as
>> checking maximum size of cmd and cmd_buf.
> I don't understand/see the issue here. Could you point out for which
> input of the set_cmdlist ioctl you see this particular overflow?
> 
> In particular it is not clear to me which size field you're talking
> about. struct g2d_cmdlist does not have any field named 'size'.
> 

I mean size of cmdlist is
size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
in exynos_g2d_set_cmdlist_ioctl().

You can reproduce overflow of size easily if you use value like
4294967295 or 2147483647 at the req->cmd_buf_nr.

Thanks.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Tobias Jakobi]

Hello Joonyoung,

Joonyoung Shim wrote:
> Hi Tobias,
> 
> On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
>> Joonyoung Shim wrote:
>>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>>> cmd_buf that has too big size. This patch will fix overflow issue as
>>> checking maximum size of cmd and cmd_buf.
>> I don't understand/see the issue here. Could you point out for which
>> input of the set_cmdlist ioctl you see this particular overflow?
>>
>> In particular it is not clear to me which size field you're talking
>> about. struct g2d_cmdlist does not have any field named 'size'.
>>
> 
> I mean size of cmdlist is
> size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
> in exynos_g2d_set_cmdlist_ioctl().
ok, that makes things more clear. But then you need to fix the commit
message. The current message implies that this 'size' you're talking
about is some property of the cmdlist.

Also the new comment is wrong.
/* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */

What is cmd and cmdlist? You're mixing two different things here. We are
still checking the size of 'cmdlist' (which is a struct g2d_cmdlist) here.

What you add is a check for the fields of 'req' (which is a struct
drm_exynos_g2d_set_cmdlist).

With all that said, I don't like the changes. I see the issue, but the
current solution should be cleaner.

I propose this. We just check req->cmd_buf_nr and req->cmd_nr against
G2D_CMDLIST_DATA_NUM. This leaves us enough headrom so that the later
computation (i.e. what is ending up in the local variable 'size') can
never overflow.

For a comment for the check I propose this:
"To avoid an integer overflow for the later size computations, we
enforce a maximum number of submitted commands here. This limit is
sufficient for all conceivable usage cases of the G2D."



With best wishes,
Tobias


> 
> You can reproduce overflow of size easily if you use value like
> 4294967295 or 2147483647 at the req->cmd_buf_nr.
> 
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Joonyoung Shim]

Hi Tobias,

On 01/19/2017 10:16 PM, Tobias Jakobi wrote:
> Hello Joonyoung,
> 
> Joonyoung Shim wrote:
>> Hi Tobias,
>>
>> On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
>>> Joonyoung Shim wrote:
>>>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>>>> cmd_buf that has too big size. This patch will fix overflow issue as
>>>> checking maximum size of cmd and cmd_buf.
>>> I don't understand/see the issue here. Could you point out for which
>>> input of the set_cmdlist ioctl you see this particular overflow?
>>>
>>> In particular it is not clear to me which size field you're talking
>>> about. struct g2d_cmdlist does not have any field named 'size'.
>>>
>>
>> I mean size of cmdlist is
>> size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
>> in exynos_g2d_set_cmdlist_ioctl().
> ok, that makes things more clear. But then you need to fix the commit
> message. The current message implies that this 'size' you're talking
> about is some property of the cmdlist.
> 
> Also the new comment is wrong.
> /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
> 
> What is cmd and cmdlist? You're mixing two different things here. We are
> still checking the size of 'cmdlist' (which is a struct g2d_cmdlist) here.
> 
> What you add is a check for the fields of 'req' (which is a struct
> drm_exynos_g2d_set_cmdlist).
> 
> With all that said, I don't like the changes. I see the issue, but the
> current solution should be cleaner.
> 
> I propose this. We just check req->cmd_buf_nr and req->cmd_nr against
> G2D_CMDLIST_DATA_NUM. This leaves us enough headrom so that the later
> computation (i.e. what is ending up in the local variable 'size') can
> never overflow.
> 

Agree, it's more clear to check req->cmd_buf_nr and req->cmd_nr against
G2D_CMDLIST_DATA_NUM.

> For a comment for the check I propose this:
> "To avoid an integer overflow for the later size computations, we
> enforce a maximum number of submitted commands here. This limit is
> sufficient for all conceivable usage cases of the G2D."
> 

Could you post your patch to ML about this if you want?

Thanks.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Tobias Jakobi]

Hello Joonyoung,


Joonyoung Shim wrote:
> Hi Tobias,
> 
> On 01/19/2017 10:16 PM, Tobias Jakobi wrote:
>> Hello Joonyoung,
>>
>> Joonyoung Shim wrote:
>>> Hi Tobias,
>>>
>>> On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
>>>> Joonyoung Shim wrote:
>>>>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>>>>> cmd_buf that has too big size. This patch will fix overflow issue as
>>>>> checking maximum size of cmd and cmd_buf.
>>>> I don't understand/see the issue here. Could you point out for which
>>>> input of the set_cmdlist ioctl you see this particular overflow?
>>>>
>>>> In particular it is not clear to me which size field you're talking
>>>> about. struct g2d_cmdlist does not have any field named 'size'.
>>>>
>>>
>>> I mean size of cmdlist is
>>> size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
>>> in exynos_g2d_set_cmdlist_ioctl().
>> ok, that makes things more clear. But then you need to fix the commit
>> message. The current message implies that this 'size' you're talking
>> about is some property of the cmdlist.
>>
>> Also the new comment is wrong.
>> /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
>>
>> What is cmd and cmdlist? You're mixing two different things here. We are
>> still checking the size of 'cmdlist' (which is a struct g2d_cmdlist) here.
>>
>> What you add is a check for the fields of 'req' (which is a struct
>> drm_exynos_g2d_set_cmdlist).
>>
>> With all that said, I don't like the changes. I see the issue, but the
>> current solution should be cleaner.
>>
>> I propose this. We just check req->cmd_buf_nr and req->cmd_nr against
>> G2D_CMDLIST_DATA_NUM. This leaves us enough headrom so that the later
>> computation (i.e. what is ending up in the local variable 'size') can
>> never overflow.
>>
> 
> Agree, it's more clear to check req->cmd_buf_nr and req->cmd_nr against
> G2D_CMDLIST_DATA_NUM.
> 
>> For a comment for the check I propose this:
>> "To avoid an integer overflow for the later size computations, we
>> enforce a maximum number of submitted commands here. This limit is
>> sufficient for all conceivable usage cases of the G2D."
>>
> 
> Could you post your patch to ML about this if you want?
Sure, I've just send it together with two other small patches. Let me
know if the current version is OK with you. I hope I did the order of
SoB correctly (I know that Krzysztof has pointed this out lately).

- Tobias


> 
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Joonyoung Shim]

Hi Tobias,

On 01/21/2017 01:05 AM, Tobias Jakobi wrote:
> Hello Joonyoung,
> 
> 
> Joonyoung Shim wrote:
>> Hi Tobias,
>>
>> On 01/19/2017 10:16 PM, Tobias Jakobi wrote:
>>> Hello Joonyoung,
>>>
>>> Joonyoung Shim wrote:
>>>> Hi Tobias,
>>>>
>>>> On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
>>>>> Joonyoung Shim wrote:
>>>>>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>>>>>> cmd_buf that has too big size. This patch will fix overflow issue as
>>>>>> checking maximum size of cmd and cmd_buf.
>>>>> I don't understand/see the issue here. Could you point out for which
>>>>> input of the set_cmdlist ioctl you see this particular overflow?
>>>>>
>>>>> In particular it is not clear to me which size field you're talking
>>>>> about. struct g2d_cmdlist does not have any field named 'size'.
>>>>>
>>>>
>>>> I mean size of cmdlist is
>>>> size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
>>>> in exynos_g2d_set_cmdlist_ioctl().
>>> ok, that makes things more clear. But then you need to fix the commit
>>> message. The current message implies that this 'size' you're talking
>>> about is some property of the cmdlist.
>>>
>>> Also the new comment is wrong.
>>> /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
>>>
>>> What is cmd and cmdlist? You're mixing two different things here. We are
>>> still checking the size of 'cmdlist' (which is a struct g2d_cmdlist) here.
>>>
>>> What you add is a check for the fields of 'req' (which is a struct
>>> drm_exynos_g2d_set_cmdlist).
>>>
>>> With all that said, I don't like the changes. I see the issue, but the
>>> current solution should be cleaner.
>>>
>>> I propose this. We just check req->cmd_buf_nr and req->cmd_nr against
>>> G2D_CMDLIST_DATA_NUM. This leaves us enough headrom so that the later
>>> computation (i.e. what is ending up in the local variable 'size') can
>>> never overflow.
>>>
>>
>> Agree, it's more clear to check req->cmd_buf_nr and req->cmd_nr against
>> G2D_CMDLIST_DATA_NUM.
>>
>>> For a comment for the check I propose this:
>>> "To avoid an integer overflow for the later size computations, we
>>> enforce a maximum number of submitted commands here. This limit is
>>> sufficient for all conceivable usage cases of the G2D."
>>>
>>
>> Could you post your patch to ML about this if you want?
> Sure, I've just send it together with two other small patches. Let me
> know if the current version is OK with you. I hope I did the order of
> SoB correctly (I know that Krzysztof has pointed this out lately).
> 

I don't know exactly about order of SoB but it's ok to me except
WARNING: line over 80 characters from checkpatch.pl.

Thanks for posting.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Inki Dae]

2017년 01월 23일 17:22에 Joonyoung Shim 이(가) 쓴 글:
> Hi Tobias,
> 
> On 01/21/2017 01:05 AM, Tobias Jakobi wrote:
>> Hello Joonyoung,
>>
>>
>> Joonyoung Shim wrote:
>>> Hi Tobias,
>>>
>>> On 01/19/2017 10:16 PM, Tobias Jakobi wrote:
>>>> Hello Joonyoung,
>>>>
>>>> Joonyoung Shim wrote:
>>>>> Hi Tobias,
>>>>>
>>>>> On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
>>>>>> Joonyoung Shim wrote:
>>>>>>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>>>>>>> cmd_buf that has too big size. This patch will fix overflow issue as
>>>>>>> checking maximum size of cmd and cmd_buf.
>>>>>> I don't understand/see the issue here. Could you point out for which
>>>>>> input of the set_cmdlist ioctl you see this particular overflow?
>>>>>>
>>>>>> In particular it is not clear to me which size field you're talking
>>>>>> about. struct g2d_cmdlist does not have any field named 'size'.
>>>>>>
>>>>>
>>>>> I mean size of cmdlist is
>>>>> size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
>>>>> in exynos_g2d_set_cmdlist_ioctl().
>>>> ok, that makes things more clear. But then you need to fix the commit
>>>> message. The current message implies that this 'size' you're talking
>>>> about is some property of the cmdlist.
>>>>
>>>> Also the new comment is wrong.
>>>> /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
>>>>
>>>> What is cmd and cmdlist? You're mixing two different things here. We are
>>>> still checking the size of 'cmdlist' (which is a struct g2d_cmdlist) here.
>>>>
>>>> What you add is a check for the fields of 'req' (which is a struct
>>>> drm_exynos_g2d_set_cmdlist).
>>>>
>>>> With all that said, I don't like the changes. I see the issue, but the
>>>> current solution should be cleaner.
>>>>
>>>> I propose this. We just check req->cmd_buf_nr and req->cmd_nr against
>>>> G2D_CMDLIST_DATA_NUM. This leaves us enough headrom so that the later
>>>> computation (i.e. what is ending up in the local variable 'size') can
>>>> never overflow.
>>>>
>>>
>>> Agree, it's more clear to check req->cmd_buf_nr and req->cmd_nr against
>>> G2D_CMDLIST_DATA_NUM.
>>>
>>>> For a comment for the check I propose this:
>>>> "To avoid an integer overflow for the later size computations, we
>>>> enforce a maximum number of submitted commands here. This limit is
>>>> sufficient for all conceivable usage cases of the G2D."
>>>>
>>>
>>> Could you post your patch to ML about this if you want?
>> Sure, I've just send it together with two other small patches. Let me
>> know if the current version is OK with you. I hope I did the order of
>> SoB correctly (I know that Krzysztof has pointed this out lately).
>>
> 
> I don't know exactly about order of SoB but it's ok to me except
> WARNING: line over 80 characters from checkpatch.pl.

Trivial thing. I can fix it.

Thanks.

> 
> Thanks for posting.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size

[Tobias Jakobi]

Joonyoung Shim wrote:
> Hi Tobias,
> 
> On 01/21/2017 01:05 AM, Tobias Jakobi wrote:
>> Hello Joonyoung,
>>
>>
>> Joonyoung Shim wrote:
>>> Hi Tobias,
>>>
>>> On 01/19/2017 10:16 PM, Tobias Jakobi wrote:
>>>> Hello Joonyoung,
>>>>
>>>> Joonyoung Shim wrote:
>>>>> Hi Tobias,
>>>>>
>>>>> On 01/17/2017 11:24 PM, Tobias Jakobi wrote:
>>>>>> Joonyoung Shim wrote:
>>>>>>> The size of cmdlist is integer type, so it can be overflowed by cmd and
>>>>>>> cmd_buf that has too big size. This patch will fix overflow issue as
>>>>>>> checking maximum size of cmd and cmd_buf.
>>>>>> I don't understand/see the issue here. Could you point out for which
>>>>>> input of the set_cmdlist ioctl you see this particular overflow?
>>>>>>
>>>>>> In particular it is not clear to me which size field you're talking
>>>>>> about. struct g2d_cmdlist does not have any field named 'size'.
>>>>>>
>>>>>
>>>>> I mean size of cmdlist is
>>>>> size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2;
>>>>> in exynos_g2d_set_cmdlist_ioctl().
>>>> ok, that makes things more clear. But then you need to fix the commit
>>>> message. The current message implies that this 'size' you're talking
>>>> about is some property of the cmdlist.
>>>>
>>>> Also the new comment is wrong.
>>>> /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */
>>>>
>>>> What is cmd and cmdlist? You're mixing two different things here. We are
>>>> still checking the size of 'cmdlist' (which is a struct g2d_cmdlist) here.
>>>>
>>>> What you add is a check for the fields of 'req' (which is a struct
>>>> drm_exynos_g2d_set_cmdlist).
>>>>
>>>> With all that said, I don't like the changes. I see the issue, but the
>>>> current solution should be cleaner.
>>>>
>>>> I propose this. We just check req->cmd_buf_nr and req->cmd_nr against
>>>> G2D_CMDLIST_DATA_NUM. This leaves us enough headrom so that the later
>>>> computation (i.e. what is ending up in the local variable 'size') can
>>>> never overflow.
>>>>
>>>
>>> Agree, it's more clear to check req->cmd_buf_nr and req->cmd_nr against
>>> G2D_CMDLIST_DATA_NUM.
>>>
>>>> For a comment for the check I propose this:
>>>> "To avoid an integer overflow for the later size computations, we
>>>> enforce a maximum number of submitted commands here. This limit is
>>>> sufficient for all conceivable usage cases of the G2D."
>>>>
>>>
>>> Could you post your patch to ML about this if you want?
>> Sure, I've just send it together with two other small patches. Let me
>> know if the current version is OK with you. I hope I did the order of
>> SoB correctly (I know that Krzysztof has pointed this out lately).
>>
> 
> I don't know exactly about order of SoB but it's ok to me except
> WARNING: line over 80 characters from checkpatch.pl.
Thanks for checking! I guess I should accustom myself to using checkpath
more regularly.

- Tobias


> 
> Thanks for posting.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>