Since the encoders have been devm-allocated, they will be freed way
before drm_mode_config_cleanup() is called. To avoid use-after-free
conditions, we then must ensure that drm_encoder_cleanup() is called
before the encoders are freed.
v2: Use the new __drmm_simple_encoder_alloc() function
Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
Cc: <[email protected]> # 5.8+
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
Use the V1 of this patch to fix v5.11 and older kernels. This V2 only
applies on the current drm-misc-next branch.
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 7bb31fbee29d..158433b4c084 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
bridge = devm_drm_panel_bridge_add_typed(dev, panel,
DRM_MODE_CONNECTOR_DPI);
- encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
- if (!encoder)
- return -ENOMEM;
+ encoder = __drmm_simple_encoder_alloc(drm, sizeof(*encoder), 0,
+ DRM_MODE_ENCODER_DPI);
+ if (IS_ERR(encoder)) {
+ ret = PTR_ERR(encoder);
+ dev_err(dev, "Failed to init encoder: %d\n", ret);
+ return ret;
+ }
encoder->possible_crtcs = 1;
drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
- ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
- if (ret) {
- dev_err(dev, "Failed to init encoder: %d\n", ret);
- return ret;
- }
-
ret = drm_bridge_attach(encoder, bridge, NULL, 0);
if (ret) {
dev_err(dev, "Unable to attach bridge\n");
--
2.29.2
Le mer. 20 janv. 2021 ? 14:01, Daniel Vetter <[email protected]> a
?crit :
> On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <[email protected]>
> wrote:
>>
>> Since the encoders have been devm-allocated, they will be freed way
>> before drm_mode_config_cleanup() is called. To avoid use-after-free
>> conditions, we then must ensure that drm_encoder_cleanup() is called
>> before the encoders are freed.
>>
>> v2: Use the new __drmm_simple_encoder_alloc() function
>>
>> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
>> Cc: <[email protected]> # 5.8+
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>>
>> Notes:
>> Use the V1 of this patch to fix v5.11 and older kernels. This
>> V2 only
>> applies on the current drm-misc-next branch.
>>
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
>> 1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index 7bb31fbee29d..158433b4c084 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device
>> *dev, bool has_components)
>> bridge =
>> devm_drm_panel_bridge_add_typed(dev, panel,
>>
>> DRM_MODE_CONNECTOR_DPI);
>>
>> - encoder = devm_kzalloc(dev, sizeof(*encoder),
>> GFP_KERNEL);
>> - if (!encoder)
>> - return -ENOMEM;
>> + encoder = __drmm_simple_encoder_alloc(drm,
>> sizeof(*encoder), 0,
>
> Please don't use the __ prefixed functions, those are the internal
> ones. The official one comes with type checking and all that included.
> Otherwise lgtm.
> -Daniel
The non-prefixed one assumes that I want to allocate a struct that
contains the encoder, not just the drm_encoder itself.
-Paul
>> +
>> DRM_MODE_ENCODER_DPI);
>> + if (IS_ERR(encoder)) {
>> + ret = PTR_ERR(encoder);
>> + dev_err(dev, "Failed to init encoder:
>> %d\n", ret);
>> + return ret;
>> + }
>>
>> encoder->possible_crtcs = 1;
>>
>> drm_encoder_helper_add(encoder,
>> &ingenic_drm_encoder_helper_funcs);
>>
>> - ret = drm_simple_encoder_init(drm, encoder,
>> DRM_MODE_ENCODER_DPI);
>> - if (ret) {
>> - dev_err(dev, "Failed to init encoder:
>> %d\n", ret);
>> - return ret;
>> - }
>> -
>> ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>> if (ret) {
>> dev_err(dev, "Unable to attach bridge\n");
>> --
>> 2.29.2
>>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <[email protected]> wrote:
>
> Since the encoders have been devm-allocated, they will be freed way
> before drm_mode_config_cleanup() is called. To avoid use-after-free
> conditions, we then must ensure that drm_encoder_cleanup() is called
> before the encoders are freed.
>
> v2: Use the new __drmm_simple_encoder_alloc() function
>
> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> Cc: <[email protected]> # 5.8+
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
>
> Notes:
> Use the V1 of this patch to fix v5.11 and older kernels. This V2 only
> applies on the current drm-misc-next branch.
>
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 7bb31fbee29d..158433b4c084 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
> bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> DRM_MODE_CONNECTOR_DPI);
>
> - encoder = devm_kzalloc(dev, sizeof(*encoder), GFP_KERNEL);
> - if (!encoder)
> - return -ENOMEM;
> + encoder = __drmm_simple_encoder_alloc(drm, sizeof(*encoder), 0,
Please don't use the __ prefixed functions, those are the internal
ones. The official one comes with type checking and all that included.
Otherwise lgtm.
-Daniel
> + DRM_MODE_ENCODER_DPI);
> + if (IS_ERR(encoder)) {
> + ret = PTR_ERR(encoder);
> + dev_err(dev, "Failed to init encoder: %d\n", ret);
> + return ret;
> + }
>
> encoder->possible_crtcs = 1;
>
> drm_encoder_helper_add(encoder, &ingenic_drm_encoder_helper_funcs);
>
> - ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DPI);
> - if (ret) {
> - dev_err(dev, "Failed to init encoder: %d\n", ret);
> - return ret;
> - }
> -
> ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> if (ret) {
> dev_err(dev, "Unable to attach bridge\n");
> --
> 2.29.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Le mer. 20 janv. 2021 ? 15:04, Daniel Vetter <[email protected]> a
?crit :
> On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <[email protected]>
> wrote:
>>
>>
>>
>> Le mer. 20 janv. 2021 ? 14:01, Daniel Vetter <[email protected]> a
>> ?crit :
>> > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil
>> <[email protected]>
>> > wrote:
>> >>
>> >> Since the encoders have been devm-allocated, they will be freed
>> way
>> >> before drm_mode_config_cleanup() is called. To avoid
>> use-after-free
>> >> conditions, we then must ensure that drm_encoder_cleanup() is
>> called
>> >> before the encoders are freed.
>> >>
>> >> v2: Use the new __drmm_simple_encoder_alloc() function
>> >>
>> >> Fixes: c369cb27c267 ("drm/ingenic: Support multiple
>> panels/bridges")
>> >> Cc: <[email protected]> # 5.8+
>> >> Signed-off-by: Paul Cercueil <[email protected]>
>> >> ---
>> >>
>> >> Notes:
>> >> Use the V1 of this patch to fix v5.11 and older kernels.
>> This
>> >> V2 only
>> >> applies on the current drm-misc-next branch.
>> >>
>> >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
>> >> 1 file changed, 7 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> >> index 7bb31fbee29d..158433b4c084 100644
>> >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> >> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct
>> device
>> >> *dev, bool has_components)
>> >> bridge =
>> >> devm_drm_panel_bridge_add_typed(dev, panel,
>> >>
>> >> DRM_MODE_CONNECTOR_DPI);
>> >>
>> >> - encoder = devm_kzalloc(dev, sizeof(*encoder),
>> >> GFP_KERNEL);
>> >> - if (!encoder)
>> >> - return -ENOMEM;
>> >> + encoder = __drmm_simple_encoder_alloc(drm,
>> >> sizeof(*encoder), 0,
>> >
>> > Please don't use the __ prefixed functions, those are the internal
>> > ones. The official one comes with type checking and all that
>> included.
>> > Otherwise lgtm.
>> > -Daniel
>>
>> The non-prefixed one assumes that I want to allocate a struct that
>> contains the encoder, not just the drm_encoder itself.
>
> Hm, but using the internal one is also a bit too ugly. A
> drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right
> thing here I think? Setting the offsets and struct sizes directly in
> these in drivers really doesn't feel like a good idea. I think simple
> encoder is the only case where we really have a need for a
> non-embeddable struct.
> -Daniel
Alright, I will add a wrapper.
Cheers,
-Paul
>>
>> >> +
>> >> DRM_MODE_ENCODER_DPI);
>> >> + if (IS_ERR(encoder)) {
>> >> + ret = PTR_ERR(encoder);
>> >> + dev_err(dev, "Failed to init encoder:
>> >> %d\n", ret);
>> >> + return ret;
>> >> + }
>> >>
>> >> encoder->possible_crtcs = 1;
>> >>
>> >> drm_encoder_helper_add(encoder,
>> >> &ingenic_drm_encoder_helper_funcs);
>> >>
>> >> - ret = drm_simple_encoder_init(drm, encoder,
>> >> DRM_MODE_ENCODER_DPI);
>> >> - if (ret) {
>> >> - dev_err(dev, "Failed to init encoder:
>> >> %d\n", ret);
>> >> - return ret;
>> >> - }
>> >> -
>> >> ret = drm_bridge_attach(encoder, bridge, NULL,
>> 0);
>> >> if (ret) {
>> >> dev_err(dev, "Unable to attach
>> bridge\n");
>> >> --
>> >> 2.29.2
>> >>
>> >
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>>
>>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On Wed, Jan 20, 2021 at 2:21 PM Paul Cercueil <[email protected]> wrote:
>
>
>
> Le mer. 20 janv. 2021 à 14:01, Daniel Vetter <[email protected]> a
> écrit :
> > On Wed, Jan 20, 2021 at 1:36 PM Paul Cercueil <[email protected]>
> > wrote:
> >>
> >> Since the encoders have been devm-allocated, they will be freed way
> >> before drm_mode_config_cleanup() is called. To avoid use-after-free
> >> conditions, we then must ensure that drm_encoder_cleanup() is called
> >> before the encoders are freed.
> >>
> >> v2: Use the new __drmm_simple_encoder_alloc() function
> >>
> >> Fixes: c369cb27c267 ("drm/ingenic: Support multiple panels/bridges")
> >> Cc: <[email protected]> # 5.8+
> >> Signed-off-by: Paul Cercueil <[email protected]>
> >> ---
> >>
> >> Notes:
> >> Use the V1 of this patch to fix v5.11 and older kernels. This
> >> V2 only
> >> applies on the current drm-misc-next branch.
> >>
> >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 16 +++++++---------
> >> 1 file changed, 7 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> index 7bb31fbee29d..158433b4c084 100644
> >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> >> @@ -1014,20 +1014,18 @@ static int ingenic_drm_bind(struct device
> >> *dev, bool has_components)
> >> bridge =
> >> devm_drm_panel_bridge_add_typed(dev, panel,
> >>
> >> DRM_MODE_CONNECTOR_DPI);
> >>
> >> - encoder = devm_kzalloc(dev, sizeof(*encoder),
> >> GFP_KERNEL);
> >> - if (!encoder)
> >> - return -ENOMEM;
> >> + encoder = __drmm_simple_encoder_alloc(drm,
> >> sizeof(*encoder), 0,
> >
> > Please don't use the __ prefixed functions, those are the internal
> > ones. The official one comes with type checking and all that included.
> > Otherwise lgtm.
> > -Daniel
>
> The non-prefixed one assumes that I want to allocate a struct that
> contains the encoder, not just the drm_encoder itself.
Hm, but using the internal one is also a bit too ugly. A
drm_plain_simple_enocder_alloc(drm, type) wrapper would be the right
thing here I think? Setting the offsets and struct sizes directly in
these in drivers really doesn't feel like a good idea. I think simple
encoder is the only case where we really have a need for a
non-embeddable struct.
-Daniel
>
> -Paul
>
> >> +
> >> DRM_MODE_ENCODER_DPI);
> >> + if (IS_ERR(encoder)) {
> >> + ret = PTR_ERR(encoder);
> >> + dev_err(dev, "Failed to init encoder:
> >> %d\n", ret);
> >> + return ret;
> >> + }
> >>
> >> encoder->possible_crtcs = 1;
> >>
> >> drm_encoder_helper_add(encoder,
> >> &ingenic_drm_encoder_helper_funcs);
> >>
> >> - ret = drm_simple_encoder_init(drm, encoder,
> >> DRM_MODE_ENCODER_DPI);
> >> - if (ret) {
> >> - dev_err(dev, "Failed to init encoder:
> >> %d\n", ret);
> >> - return ret;
> >> - }
> >> -
> >> ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> >> if (ret) {
> >> dev_err(dev, "Unable to attach bridge\n");
> >> --
> >> 2.29.2
> >>
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch