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