[PATCH 0/5] fix missing break in list_or_each_entry

[PATCH 0/5] fix missing break in list_or_each_entry

[Xiaomeng Tong]

Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could lead to invalid reference or set 'is_*' flags mistakely.

To fix this, when an entry is found, add a break to exit the loop.

Xiaomeng Tong (5):
  gma500: fix a missing break in oaktrail_crtc_mode_set
  gma500: fix a missing break in cdv_intel_crtc_mode_set
  gma500: fix a missing break in psb_intel_crtc_mode_set
  gma500: fix a missing break in cdv_intel_dp_set_m_n
  gma500: fix a missing break in psb_driver_load

 drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++
 drivers/gpu/drm/gma500/cdv_intel_dp.c      | 2 ++
 drivers/gpu/drm/gma500/oaktrail_crtc.c     | 2 ++
 drivers/gpu/drm/gma500/psb_drv.c           | 2 ++
 drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++
 5 files changed, 10 insertions(+)

-- 
2.17.1

[PATCH 1/5] gma500: fix a missing break in oaktrail_crtc_mode_set

[Xiaomeng Tong]

Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
will certainly lead to a invalid reference to list itereator variable
'connector' after the loop pointing an bogus address at an offset
from the list head, and could lead to multiple 'is_*' flags being set
with true mistakely too.

The invalid reference to list itereator is here:
	drm_object_property_get_value(&connector->base,

To fix this, when found the entry, add a break after the switch
statement.

Fixes: a69ac9ea85d87 ("drm/gma500: drm_connector_property -> drm_object_property")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
index 36c7c2686c90..eb2d79872bd5 100644
--- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
+++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
@@ -409,6 +409,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
 			is_mipi = true;
 			break;
 		}
+
+		break;
 	}
 
 	/* Disable the VGA plane that we never use */
-- 
2.17.1

Re: [PATCH 1/5] gma500: fix a missing break in oaktrail_crtc_mode_set

[Patrik Jakobsson]

On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> Instead of exiting the loop as expected when an entry is found, the
> list_for_each_entry() continues until the traversal is complete. It
> will certainly lead to a invalid reference to list itereator variable
> 'connector' after the loop pointing an bogus address at an offset
> from the list head, and could lead to multiple 'is_*' flags being set
> with true mistakely too.
>
> The invalid reference to list itereator is here:
>         drm_object_property_get_value(&connector->base,
>
> To fix this, when found the entry, add a break after the switch
> statement.

Hi, this is already fixed in:
commit b1a7d0ddb169774c3db5afe9e64124daea7fdd9f
Author: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Date:   Tue Mar 22 14:17:38 2022 +0100

    drm/gma500: Make use of the drm connector iterator

>
> Fixes: a69ac9ea85d87 ("drm/gma500: drm_connector_property -> drm_object_property")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>  drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> index 36c7c2686c90..eb2d79872bd5 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> @@ -409,6 +409,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
>                         is_mipi = true;
>                         break;
>                 }
> +
> +               break;
>         }
>
>         /* Disable the VGA plane that we never use */
> --
> 2.17.1
>

[PATCH 2/5] gma500: fix a missing break in cdv_intel_crtc_mode_set

[Xiaomeng Tong]

Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could lead to a invalid reference to 'ddi_select' after the loop, and
could lead to multiple 'is_*' flags being set with true mistakely, too.

The invalid reference to 'ddi_select' is here:
	cdv_dpll_set_clock_cdv(dev, crtc, &clock, is_lvds, ddi_select);

To fix this, when found the entry, add a break after the switch statement.

Fixes: d66760962d75 ("gma500: Program the DPLL lane based on the selected digitial port")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c b/drivers/gpu/drm/gma500/cdv_intel_display.c
index 94ebc48a4349..3e93019b17cb 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_display.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
@@ -616,6 +616,8 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
 			DRM_ERROR("invalid output type.\n");
 			return 0;
 		}
+
+		break;
 	}
 
 	if (dev_priv->dplla_96mhz)
-- 
2.17.1

Re: [PATCH 2/5] gma500: fix a missing break in cdv_intel_crtc_mode_set

[Patrik Jakobsson]

On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> Instead of exiting the loop as expected when an entry is found, the
> list_for_each_entry() continues until the traversal is complete. It
> could lead to a invalid reference to 'ddi_select' after the loop, and
> could lead to multiple 'is_*' flags being set with true mistakely, too.
>
> The invalid reference to 'ddi_select' is here:
>         cdv_dpll_set_clock_cdv(dev, crtc, &clock, is_lvds, ddi_select);
>
> To fix this, when found the entry, add a break after the switch statement.
>
> Fixes: d66760962d75 ("gma500: Program the DPLL lane based on the selected digitial port")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>

Hi, this one is also already fixed in:

commit b1a7d0ddb169774c3db5afe9e64124daea7fdd9f
Author: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Date:   Tue Mar 22 14:17:38 2022 +0100

    drm/gma500: Make use of the drm connector iterator

> ---
>  drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c b/drivers/gpu/drm/gma500/cdv_intel_display.c
> index 94ebc48a4349..3e93019b17cb 100644
> --- a/drivers/gpu/drm/gma500/cdv_intel_display.c
> +++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
> @@ -616,6 +616,8 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
>                         DRM_ERROR("invalid output type.\n");
>                         return 0;
>                 }
> +
> +               break;
>         }
>
>         if (dev_priv->dplla_96mhz)
> --
> 2.17.1
>

[PATCH 3/5] gma500: fix a missing break in psb_intel_crtc_mode_set

[Xiaomeng Tong]

Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could result in multiple 'is_*' flags being set with true mistakely.

To fix this, when found the entry, add a break after the switch
statement.

Fixes: 89c78134cc54d (" gma500: Add Poulsbo support")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index 42d1a733e124..85fc61bf333a 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -134,6 +134,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
 			is_tv = true;
 			break;
 		}
+
+		break;
 	}
 
 	refclk = 96000;
-- 
2.17.1

Re: [PATCH 3/5] gma500: fix a missing break in psb_intel_crtc_mode_set

[Patrik Jakobsson]

On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> Instead of exiting the loop as expected when an entry is found, the
> list_for_each_entry() continues until the traversal is complete. It
> could result in multiple 'is_*' flags being set with true mistakely.
>
> To fix this, when found the entry, add a break after the switch
> statement.
>
> Fixes: 89c78134cc54d (" gma500: Add Poulsbo support")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>

This patch doesn't apply for me and needs to be rebased on top of
drm-misc-next or drm-tip.

On Poulsbo there should only be one encoder per crtc so this is only a
theoretical issue. But it is good practice to exit the loop early if
we can so the patch still makes sense.

Also, please use the correct subject prefix: drm/gma500: instead of
just gma500:.

Thanks
Patrik

> ---
>  drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index 42d1a733e124..85fc61bf333a 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -134,6 +134,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
>                         is_tv = true;
>                         break;
>                 }
> +
> +               break;
>         }
>
>         refclk = 96000;
> --
> 2.17.1
>

Re: [PATCH 3/5] gma500: fix a missing break in psb_intel_crtc_mode_set

[Xiaomeng Tong]

> On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
> >
> > Instead of exiting the loop as expected when an entry is found, the
> > list_for_each_entry() continues until the traversal is complete. It
> > could result in multiple 'is_*' flags being set with true mistakely.
> >
> > To fix this, when found the entry, add a break after the switch
> > statement.
> >
> > Fixes: 89c78134cc54d (" gma500: Add Poulsbo support")
> > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> 
> This patch doesn't apply for me and needs to be rebased on top of
> drm-misc-next or drm-tip.
> 
> On Poulsbo there should only be one encoder per crtc so this is only a
> theoretical issue. But it is good practice to exit the loop early if
> we can so the patch still makes sense.
> 
> Also, please use the correct subject prefix: drm/gma500: instead of
> just gma500:.

I will resend another patch as you suggested, thank you.

--
Xiaomeng Tong

[PATCH 4/5] gma500: fix a missing break in cdv_intel_dp_set_m_n

[Xiaomeng Tong]

Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could lead to a invalid reference to 'lane_count/bpp' after the loop.

The invalid reference to 'lane_count/bpp' is here:
	cdv_intel_dp_compute_m_n(bpp, lane_count,

To fix this, when found the entry, add a break after the switch statement.

Fixes: 8695b61294356 ("gma500: Add the support of display port on CDV")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c
index ba6ad1466374..e6473b8da296 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
@@ -1016,6 +1016,8 @@ cdv_intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 			bpp = dev_priv->edp.bpp;
 			break;
 		}
+
+		break;
 	}
 
 	/*
-- 
2.17.1

Re: [PATCH 4/5] gma500: fix a missing break in cdv_intel_dp_set_m_n

[Patrik Jakobsson]

On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> Instead of exiting the loop as expected when an entry is found, the
> list_for_each_entry() continues until the traversal is complete. It
> could lead to a invalid reference to 'lane_count/bpp' after the loop.
>
> The invalid reference to 'lane_count/bpp' is here:
>         cdv_intel_dp_compute_m_n(bpp, lane_count,
>
> To fix this, when found the entry, add a break after the switch statement.
>
> Fixes: 8695b61294356 ("gma500: Add the support of display port on CDV")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>

This loop already breaks when the desired conditions are met (see the
if statements).

> ---
>  drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c b/drivers/gpu/drm/gma500/cdv_intel_dp.c
> index ba6ad1466374..e6473b8da296 100644
> --- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
> +++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
> @@ -1016,6 +1016,8 @@ cdv_intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>                         bpp = dev_priv->edp.bpp;
>                         break;
>                 }
> +
> +               break;
>         }
>
>         /*
> --
> 2.17.1
>

[PATCH 5/5] gma500: fix a missing break in psb_driver_load

[Xiaomeng Tong]

Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. To
avoid potential executing 'ret = gma_backlight_init(dev);' repeatly,
add a break after the switch statement.

Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
---
 drivers/gpu/drm/gma500/psb_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 65cf1c79dd7c..d65a68811bf7 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 			ret = gma_backlight_init(dev);
 			break;
 		}
+
+		break;
 	}
 
 	if (ret)
-- 
2.17.1

Re: [PATCH 5/5] gma500: fix a missing break in psb_driver_load

[Patrik Jakobsson]

On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> Instead of exiting the loop as expected when an entry is found, the
> list_for_each_entry() continues until the traversal is complete. To
> avoid potential executing 'ret = gma_backlight_init(dev);' repeatly,
> add a break after the switch statement.
>
> Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>

This is incorrect. If we always break on the first iteration we will
only run gma_backlight_init() if the first connector is LVDS or MIPI.
This might not be the case and gma_backlight_init() will never run.
The other loops you have been looking at have an "if (xxx != yyy)
continue;" statement at the top which skips all the unwanted entries
but this loop does not.

> ---
>  drivers/gpu/drm/gma500/psb_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> index 65cf1c79dd7c..d65a68811bf7 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
>                         ret = gma_backlight_init(dev);
>                         break;
>                 }
> +
> +               break;
>         }
>
>         if (ret)
> --
> 2.17.1
>

Re: [PATCH 5/5] gma500: fix a missing break in psb_driver_load

[Xiaomeng Tong]

On Fri, 1 Apr 2022 12:10:48 +0200, Patrik Jakobsson wrote:
> On Wed, Mar 30, 2022 at 2:03 PM Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
> >
> > Instead of exiting the loop as expected when an entry is found, the
> > list_for_each_entry() continues until the traversal is complete. To
> > avoid potential executing 'ret = gma_backlight_init(dev);' repeatly,
> > add a break after the switch statement.
> >
> > Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers")
> > Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> 
> This is incorrect. If we always break on the first iteration we will
> only run gma_backlight_init() if the first connector is LVDS or MIPI.
> This might not be the case and gma_backlight_init() will never run.
> The other loops you have been looking at have an "if (xxx != yyy)
> continue;" statement at the top which skips all the unwanted entries
> but this loop does not.
> 

Yes, your are correct. But it still need to break the loop when found it.
So it is better to add if(!ret) break; after the switch statment.
I will resend another patch if it is necessary.

> > ---
> >  drivers/gpu/drm/gma500/psb_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
> > index 65cf1c79dd7c..d65a68811bf7 100644
> > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > @@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
> >                         ret = gma_backlight_init(dev);
> >                         break;
> >                 }
> > +
> > +               break;
> >         }
> >
> >         if (ret)
> > --
> > 2.17.1
> >

--
Xiaomeng Tong