[PATCH] drm/bridge: Add null pointer check for ITE IT6263

[PATCH] drm/bridge: Add null pointer check for ITE IT6263

[Chenyuan Yang]

drm_atomic_get_new_connector_for_encoder and
drm_atomic_get_new_connector_state could return Null.
Thus, add the null pointer check for them with a similar format with
it6505_bridge_atomic_enable in ITE IT6505.

Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
Fixes: 049723628716 ("drm/bridge: Add ITE IT6263 LVDS to HDMI converter")
---
 drivers/gpu/drm/bridge/ite-it6263.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
index a3a63a977b0a..3a20b2088bf9 100644
--- a/drivers/gpu/drm/bridge/ite-it6263.c
+++ b/drivers/gpu/drm/bridge/ite-it6263.c
@@ -590,15 +590,28 @@ static void it6263_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct drm_connector *connector;
 	bool is_stable = false;
 	struct drm_crtc *crtc;
+	struct drm_connector_state *conn_state;
 	unsigned int val;
 	bool pclk_high;
 	int i, ret;
 
 	connector = drm_atomic_get_new_connector_for_encoder(state,
 							     bridge->encoder);
-	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
+	if (WARN_ON(!connector))
+		return;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state))
+		return;
+
+	crtc = conn_state->crtc;
 	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	if (WARN_ON(!crtc_state))
+		return;
+
 	mode = &crtc_state->adjusted_mode;
+	if (WARN_ON(!mode))
+		return;
 
 	regmap_write(regmap, HDMI_REG_HDMI_MODE, TX_HDMI_MODE);
 
-- 
2.34.1

RE: [PATCH] drm/bridge: Add null pointer check for ITE IT6263

[Biju Das]

Hi Chenyuan Yang,

Thanks for the patch.

> -----Original Message-----
> From: Chenyuan Yang <chenyuan0y@gmail.com>
> Sent: 22 July 2025 21:41
> Subject: [PATCH] drm/bridge: Add null pointer check for ITE IT6263
> 
> drm_atomic_get_new_connector_for_encoder and drm_atomic_get_new_connector_state could return Null.
> Thus, add the null pointer check for them with a similar format with it6505_bridge_atomic_enable in
> ITE IT6505.
> 
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> Fixes: 049723628716 ("drm/bridge: Add ITE IT6263 LVDS to HDMI converter")

Normally Fixes should be above Sb tag.

> ---
>  drivers/gpu/drm/bridge/ite-it6263.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
> index a3a63a977b0a..3a20b2088bf9 100644
> --- a/drivers/gpu/drm/bridge/ite-it6263.c
> +++ b/drivers/gpu/drm/bridge/ite-it6263.c
> @@ -590,15 +590,28 @@ static void it6263_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector *connector;
>  	bool is_stable = false;
>  	struct drm_crtc *crtc;
> +	struct drm_connector_state *conn_state;

Please arrange it in reverse X-mas tree fashion.

>  	unsigned int val;
>  	bool pclk_high;
>  	int i, ret;
> 
>  	connector = drm_atomic_get_new_connector_for_encoder(state,
>  							     bridge->encoder);
> -	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> +	if (WARN_ON(!connector))

Why WARN_ON everywhere? Can we use dev_err instead?

Cheers,
Biju

> +		return;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	if (WARN_ON(!conn_state))
> +		return;
> +
> +	crtc = conn_state->crtc;
>  	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	if (WARN_ON(!crtc_state))
> +		return;
> +
>  	mode = &crtc_state->adjusted_mode;
> +	if (WARN_ON(!mode))
> +		return;
> 
>  	regmap_write(regmap, HDMI_REG_HDMI_MODE, TX_HDMI_MODE);
> 
> --
> 2.34.1

Re: [PATCH] drm/bridge: Add null pointer check for ITE IT6263

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

On Tue, Jul 22, 2025 at 03:41:14PM -0500, Chenyuan Yang wrote:
> drm_atomic_get_new_connector_for_encoder and
> drm_atomic_get_new_connector_state could return Null.

They can, but not in that scenario. atomic_enable will never be called
if either would return NULL.

In which situation did you trigger this bug?

> Thus, add the null pointer check for them with a similar format with
> it6505_bridge_atomic_enable in ITE IT6505.
> 
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> Fixes: 049723628716 ("drm/bridge: Add ITE IT6263 LVDS to HDMI converter")
> ---
>  drivers/gpu/drm/bridge/ite-it6263.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
> index a3a63a977b0a..3a20b2088bf9 100644
> --- a/drivers/gpu/drm/bridge/ite-it6263.c
> +++ b/drivers/gpu/drm/bridge/ite-it6263.c
> @@ -590,15 +590,28 @@ static void it6263_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector *connector;
>  	bool is_stable = false;
>  	struct drm_crtc *crtc;
> +	struct drm_connector_state *conn_state;
>  	unsigned int val;
>  	bool pclk_high;
>  	int i, ret;
>  
>  	connector = drm_atomic_get_new_connector_for_encoder(state,
>  							     bridge->encoder);
> -	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> +	if (WARN_ON(!connector))
> +		return;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	if (WARN_ON(!conn_state))
> +		return;
> +
> +	crtc = conn_state->crtc;
>  	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	if (WARN_ON(!crtc_state))
> +		return;
> +
>  	mode = &crtc_state->adjusted_mode;
> +	if (WARN_ON(!mode))
> +		return;

And that condition can never be true.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH] drm/bridge: Add null pointer check for ITE IT6263

[Chenyuan Yang]

Apologies for the second email.
I am resending this message as the formatting in the previous version
was incorrect

On Tue, Jul 22, 2025 at 11:57 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Jul 22, 2025 at 03:41:14PM -0500, Chenyuan Yang wrote:
> > drm_atomic_get_new_connector_for_encoder and
> > drm_atomic_get_new_connector_state could return Null.
>
> They can, but not in that scenario. atomic_enable will never be called
> if either would return NULL.
>
> In which situation did you trigger this bug?

This is found by our static analysis tool based on the fact that
drm_atomic_get_new_connector_state() could return NULL.
We also noticed that under the same dir, the ITE IT6505 transmitter
has such checks.
Thus, we assume it would be good to have similar checks here.

> > Thus, add the null pointer check for them with a similar format with
> > it6505_bridge_atomic_enable in ITE IT6505.
> >
> > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > Fixes: 049723628716 ("drm/bridge: Add ITE IT6263 LVDS to HDMI converter")
> > ---
> >  drivers/gpu/drm/bridge/ite-it6263.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
> > index a3a63a977b0a..3a20b2088bf9 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6263.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6263.c
> > @@ -590,15 +590,28 @@ static void it6263_bridge_atomic_enable(struct drm_bridge *bridge,
> >       struct drm_connector *connector;
> >       bool is_stable = false;
> >       struct drm_crtc *crtc;
> > +     struct drm_connector_state *conn_state;
> >       unsigned int val;
> >       bool pclk_high;
> >       int i, ret;
> >
> >       connector = drm_atomic_get_new_connector_for_encoder(state,
> >                                                            bridge->encoder);
> > -     crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> > +     if (WARN_ON(!connector))
> > +             return;
> > +
> > +     conn_state = drm_atomic_get_new_connector_state(state, connector);
> > +     if (WARN_ON(!conn_state))
> > +             return;
> > +
> > +     crtc = conn_state->crtc;
> >       crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > +     if (WARN_ON(!crtc_state))
> > +             return;
> > +
> >       mode = &crtc_state->adjusted_mode;
> > +     if (WARN_ON(!mode))
> > +             return;
>
> And that condition can never be true.
>
> Maxime

Re: [PATCH] drm/bridge: Add null pointer check for ITE IT6263

[Liu Ying]

Hi Chenyuan,

On 07/23/2025, Chenyuan Yang wrote:
> On Tue, Jul 22, 2025 at 23:57 Maxime Ripard <mripard@kernel.org> wrote:
> 
>> On Tue, Jul 22, 2025 at 03:41:14PM -0500, Chenyuan Yang wrote:
>>> drm_atomic_get_new_connector_for_encoder and
>>> drm_atomic_get_new_connector_state could return Null.
>>
>> They can, but not in that scenario. atomic_enable will never be called
>> if either would return NULL.
>>
>> In which situation did you trigger this bug?
> 
> 
> This is found by our static analysis tool based on the fact that
> drm_atomic_get_new_connector_state() could return NULL. We also noticed
> that under the same dir, ITE IT6505 transmitter has such checks. Thus, we
> assume it would be good to have similar checks here.

You need to properly triage the tool's report, which means if there is
no real case that could produce those NULL pointers, then the report
should be false-positive, otherwise show us any real case that could in
commit message.  Assuming the checks are needed is not good enough.
Please do your home work before sending the patch.

> 
> 
>>> Thus, add the null pointer check for them with a similar format with
>>> it6505_bridge_atomic_enable in ITE IT6505.
>>>
>>> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
>>> Fixes: 049723628716 ("drm/bridge: Add ITE IT6263 LVDS to HDMI converter")
>>> ---
>>>  drivers/gpu/drm/bridge/ite-it6263.c | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ite-it6263.c
>> b/drivers/gpu/drm/bridge/ite-it6263.c
>>> index a3a63a977b0a..3a20b2088bf9 100644
>>> --- a/drivers/gpu/drm/bridge/ite-it6263.c
>>> +++ b/drivers/gpu/drm/bridge/ite-it6263.c
>>> @@ -590,15 +590,28 @@ static void it6263_bridge_atomic_enable(struct
>> drm_bridge *bridge,
>>>       struct drm_connector *connector;
>>>       bool is_stable = false;
>>>       struct drm_crtc *crtc;
>>> +     struct drm_connector_state *conn_state;
>>>       unsigned int val;
>>>       bool pclk_high;
>>>       int i, ret;
>>>
>>>       connector = drm_atomic_get_new_connector_for_encoder(state,
>>>
>> bridge->encoder);
>>> -     crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
>>> +     if (WARN_ON(!connector))
>>> +             return;
>>> +
>>> +     conn_state = drm_atomic_get_new_connector_state(state, connector);
>>> +     if (WARN_ON(!conn_state))
>>> +             return;
>>> +
>>> +     crtc = conn_state->crtc;
>>>       crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>> +     if (WARN_ON(!crtc_state))
>>> +             return;
>>> +
>>>       mode = &crtc_state->adjusted_mode;
>>> +     if (WARN_ON(!mode))
>>> +             return;
>>
>> And that condition can never be true.
>>
>> Maxime
>>
> 


-- 
Regards,
Liu Ying