| Message ID | 1484628126-21671-1-git-send-email-jy0922.shim@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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"); > -- 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
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. -- 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
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 > -- 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
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. -- 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
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 > -- 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
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. -- 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
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 > > -- 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
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 > -- 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
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");
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(-)