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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
