[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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>