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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
