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

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

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