* [PATCH] drm/exynos: g2d: fix overflow of cmdlist size [not found] <CGME20170117044134epcas5p2d0e67623efa7eb0b9a1ad6f8a593f0c4@epcas5p2.samsung.com> @ 2017-01-17 4:42 ` Joonyoung Shim 2017-01-17 14:24 ` Tobias Jakobi 0 siblings, 1 reply; 9+ messages in thread From: Joonyoung Shim @ 2017-01-17 4:42 UTC (permalink / raw) To: dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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. Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c index fbd13fa..b31244f 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c @@ -1250,7 +1250,14 @@ int exynos_g2d_set_cmdlist_ioctl(struct drm_device *drm_dev, void *data, cmdlist->data[cmdlist->last++] = G2D_INTEN_ACF; } - /* Check size of cmdlist: last 2 is about G2D_BITBLT_START */ + /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */ + size = (G2D_CMDLIST_DATA_NUM - cmdlist->last - 2) / 2; + if (req->cmd_nr > size || req->cmd_buf_nr > size) { + dev_err(dev, "size of cmd or cmd_buf is too big\n"); + ret = -EINVAL; + goto err_free_event; + } + size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2; if (size > G2D_CMDLIST_DATA_NUM) { dev_err(dev, "cmdlist size is too big\n"); -- 1.9.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-17 4:42 ` [PATCH] drm/exynos: g2d: fix overflow of cmdlist size Joonyoung Shim @ 2017-01-17 14:24 ` Tobias Jakobi 2017-01-18 0:30 ` Joonyoung Shim 0 siblings, 1 reply; 9+ messages in thread From: Tobias Jakobi @ 2017-01-17 14:24 UTC (permalink / raw) To: Joonyoung Shim, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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'. With best wishes, Tobias > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_g2d.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c > index fbd13fa..b31244f 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c > @@ -1250,7 +1250,14 @@ int exynos_g2d_set_cmdlist_ioctl(struct drm_device *drm_dev, void *data, > cmdlist->data[cmdlist->last++] = G2D_INTEN_ACF; > } > > - /* Check size of cmdlist: last 2 is about G2D_BITBLT_START */ > + /* Check size of cmd and cmdlist: last 2 is about G2D_BITBLT_START */ > + size = (G2D_CMDLIST_DATA_NUM - cmdlist->last - 2) / 2; > + if (req->cmd_nr > size || req->cmd_buf_nr > size) { > + dev_err(dev, "size of cmd or cmd_buf is too big\n"); > + ret = -EINVAL; > + goto err_free_event; > + } > + > size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2; > if (size > G2D_CMDLIST_DATA_NUM) { > dev_err(dev, "cmdlist size is too big\n"); > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-17 14:24 ` Tobias Jakobi @ 2017-01-18 0:30 ` Joonyoung Shim 2017-01-19 13:16 ` Tobias Jakobi 0 siblings, 1 reply; 9+ messages in thread From: Joonyoung Shim @ 2017-01-18 0:30 UTC (permalink / raw) To: Tobias Jakobi, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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(). You can reproduce overflow of size easily if you use value like 4294967295 or 2147483647 at the req->cmd_buf_nr. Thanks. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-18 0:30 ` Joonyoung Shim @ 2017-01-19 13:16 ` Tobias Jakobi 2017-01-19 23:53 ` Joonyoung Shim 0 siblings, 1 reply; 9+ messages in thread From: Tobias Jakobi @ 2017-01-19 13:16 UTC (permalink / raw) To: Joonyoung Shim, Tobias Jakobi, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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@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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-19 13:16 ` Tobias Jakobi @ 2017-01-19 23:53 ` Joonyoung Shim 2017-01-20 16:05 ` Tobias Jakobi 0 siblings, 1 reply; 9+ messages in thread From: Joonyoung Shim @ 2017-01-19 23:53 UTC (permalink / raw) To: Tobias Jakobi, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-19 23:53 ` Joonyoung Shim @ 2017-01-20 16:05 ` Tobias Jakobi 2017-01-23 8:22 ` Joonyoung Shim 0 siblings, 1 reply; 9+ messages in thread From: Tobias Jakobi @ 2017-01-20 16:05 UTC (permalink / raw) To: Joonyoung Shim, Tobias Jakobi, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-20 16:05 ` Tobias Jakobi @ 2017-01-23 8:22 ` Joonyoung Shim 2017-01-23 9:10 ` Inki Dae 2017-01-23 12:47 ` Tobias Jakobi 0 siblings, 2 replies; 9+ messages in thread From: Joonyoung Shim @ 2017-01-23 8:22 UTC (permalink / raw) To: Tobias Jakobi, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 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 posting. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-23 8:22 ` Joonyoung Shim @ 2017-01-23 9:10 ` Inki Dae 2017-01-23 12:47 ` Tobias Jakobi 1 sibling, 0 replies; 9+ messages in thread From: Inki Dae @ 2017-01-23 9:10 UTC (permalink / raw) To: Joonyoung Shim, Tobias Jakobi, dri-devel; +Cc: linux-samsung-soc, sw0312.kim 2017년 01월 23일 17:22에 Joonyoung Shim 이(가) 쓴 글: > 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. Trivial thing. I can fix it. Thanks. > > Thanks for posting. > -- > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/exynos: g2d: fix overflow of cmdlist size 2017-01-23 8:22 ` Joonyoung Shim 2017-01-23 9:10 ` Inki Dae @ 2017-01-23 12:47 ` Tobias Jakobi 1 sibling, 0 replies; 9+ messages in thread From: Tobias Jakobi @ 2017-01-23 12:47 UTC (permalink / raw) To: Joonyoung Shim, Tobias Jakobi, dri-devel Cc: inki.dae, sw0312.kim, linux-samsung-soc 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-01-23 12:47 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20170117044134epcas5p2d0e67623efa7eb0b9a1ad6f8a593f0c4@epcas5p2.samsung.com>
2017-01-17 4:42 ` [PATCH] drm/exynos: g2d: fix overflow of cmdlist size Joonyoung Shim
2017-01-17 14:24 ` Tobias Jakobi
2017-01-18 0:30 ` Joonyoung Shim
2017-01-19 13:16 ` Tobias Jakobi
2017-01-19 23:53 ` Joonyoung Shim
2017-01-20 16:05 ` Tobias Jakobi
2017-01-23 8:22 ` Joonyoung Shim
2017-01-23 9:10 ` Inki Dae
2017-01-23 12:47 ` Tobias Jakobi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.