[PATCH] drm: bridge: it66121: Fix invalid connector dereference

[PATCH] drm: bridge: it66121: Fix invalid connector dereference

[Jai Luthra]

Fix the NULL pointer dereference when no monitor is connected, and the
sound card is opened from userspace.

Instead return an error as EDID information cannot be provided to
the sound framework if there is no connector attached.

Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
Reported-by: Nishanth Menon <nm@ti.com>
Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
Signed-off-by: Jai Luthra <j-luthra@ti.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 466641c77fe9..d6fa00dea464 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev, void *data,
 {
 	struct it66121_ctx *ctx = dev_get_drvdata(dev);
 
+	if (!ctx->connector) {
+		dev_dbg(dev, "No connector present, cannot provide EDID data");
+		return -EINVAL;
+	}
+
 	mutex_lock(&ctx->lock);
 
 	memcpy(buf, ctx->connector->eld,

---
base-commit: 6269320850097903b30be8f07a5c61d9f7592393
change-id: 20230825-it66121_edid-6ee98517808b

Best regards,
-- 
Jai Luthra <j-luthra@ti.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm: bridge: it66121: Fix invalid connector dereference

[Aradhya Bhatia]

Hi Jai,

Thanks for debugging the issue.

On 25-Aug-23 16:32, Jai Luthra wrote:
> Fix the NULL pointer dereference when no monitor is connected, and the
> sound card is opened from userspace.
> 
> Instead return an error as EDID information cannot be provided to
> the sound framework if there is no connector attached.
> 
> Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
> Reported-by: Nishanth Menon <nm@ti.com>
> Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 466641c77fe9..d6fa00dea464 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev, void *data,
>  {
>  	struct it66121_ctx *ctx = dev_get_drvdata(dev);
>  
> +	if (!ctx->connector) {
> +		dev_dbg(dev, "No connector present, cannot provide EDID data");
> +		return -EINVAL;
> +	}
> +

There are not many HDMI bridges that support codecs in the kernel, but
upon a quick look, bridge/analogix/anx7625.c and
bridge/synopsys/dw-hdmi* gracefully return a buffer of 0s when the
connector is unavailable.

I am not sure why that is done, but I also don't see the hdmi-codec
driver handle the 0s situation properly. It is business as usual for the
hdmi-codec.

Did you come across some observation when you were testing?

Regards
Aradhya

>  	mutex_lock(&ctx->lock);
>  
>  	memcpy(buf, ctx->connector->eld,
> 
> ---
> base-commit: 6269320850097903b30be8f07a5c61d9f7592393
> change-id: 20230825-it66121_edid-6ee98517808b
> 
> Best regards,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm: bridge: it66121: Fix invalid connector dereference

[Jai Luthra]

[-- Attachment #1.1: Type: text/plain, Size: 2794 bytes --]

Hi Aradhya,

Thanks for the comments.

On Aug 28, 2023 at 16:34:30 +0530, Aradhya Bhatia wrote:
> Hi Jai,
> 
> Thanks for debugging the issue.
> 
> On 25-Aug-23 16:32, Jai Luthra wrote:
> > Fix the NULL pointer dereference when no monitor is connected, and the
> > sound card is opened from userspace.
> > 
> > Instead return an error as EDID information cannot be provided to
> > the sound framework if there is no connector attached.
> > 
> > Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
> > Reported-by: Nishanth Menon <nm@ti.com>
> > Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
> > Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > ---
> >  drivers/gpu/drm/bridge/ite-it66121.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> > index 466641c77fe9..d6fa00dea464 100644
> > --- a/drivers/gpu/drm/bridge/ite-it66121.c
> > +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> > @@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev, void *data,
> >  {
> >  	struct it66121_ctx *ctx = dev_get_drvdata(dev);
> >  
> > +	if (!ctx->connector) {
> > +		dev_dbg(dev, "No connector present, cannot provide EDID data");
> > +		return -EINVAL;
> > +	}
> > +
> 
> There are not many HDMI bridges that support codecs in the kernel, but
> upon a quick look, bridge/analogix/anx7625.c and
> bridge/synopsys/dw-hdmi* gracefully return a buffer of 0s when the
> connector is unavailable.

Interesting, that sounds cleaner to me.

> 
> I am not sure why that is done, but I also don't see the hdmi-codec
> driver handle the 0s situation properly. It is business as usual for the
> hdmi-codec.

Chasing this down through the hdmi-codec driver and sound framework, it 
will use sane defaults for supported channels and sample rates if the 
actual EDID struct we are passing is all 0s. I tested it out on the 
beagle play board and did not see any crashes.

> 
> Did you come across some observation when you were testing?

Yes, with the current state of the patch, if I plug-in a monitor after 
the soundcard is probed I cannot playback audio, because we returned an 
error initially.

This issue goes away if we return a buffer of 0s like those other 
bridges. Overall I'm happy with this idea, so I will send a v2 fixing 
this.

> 
> Regards
> Aradhya
> 
> >  	mutex_lock(&ctx->lock);
> >  
> >  	memcpy(buf, ctx->connector->eld,
> > 
> > ---
> > base-commit: 6269320850097903b30be8f07a5c61d9f7592393
> > change-id: 20230825-it66121_edid-6ee98517808b
> > 
> > Best regards,
> 

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm: bridge: it66121: Fix invalid connector dereference

[Helen Mae Koike Fornazier]

On Friday, August 25, 2023 08:02 -03, Jai Luthra <j-luthra@ti.com> wrote:

> Fix the NULL pointer dereference when no monitor is connected, and the
> sound card is opened from userspace.
> 
> Instead return an error as EDID information cannot be provided to
> the sound framework if there is no connector attached.
> 
> Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
> Reported-by: Nishanth Menon <nm@ti.com>
> Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
> Signed-off-by: Jai Luthra <j-luthra@ti.com>

Reviewed-by: Helen Koike <helen.koike@collabora.com>

> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 466641c77fe9..d6fa00dea464 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev, void *data,
>  {
>  	struct it66121_ctx *ctx = dev_get_drvdata(dev);
>  
> +	if (!ctx->connector) {
> +		dev_dbg(dev, "No connector present, cannot provide EDID data");
> +		return -EINVAL;
> +	}
> +
>  	mutex_lock(&ctx->lock);
>  
>  	memcpy(buf, ctx->connector->eld,
> 
> ---
> base-commit: 6269320850097903b30be8f07a5c61d9f7592393
> change-id: 20230825-it66121_edid-6ee98517808b
> 
> Best regards,
> -- 
> Jai Luthra <j-luthra@ti.com>
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm: bridge: it66121: Fix invalid connector dereference

[Nishanth Menon]

On 16:35-20230828, Helen Mae Koike Fornazier wrote:
> On Friday, August 25, 2023 08:02 -03, Jai Luthra <j-luthra@ti.com> wrote:
> 
> > Fix the NULL pointer dereference when no monitor is connected, and the
> > sound card is opened from userspace.
> > 
> > Instead return an error as EDID information cannot be provided to
> > the sound framework if there is no connector attached.
> > 
> > Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
> > Reported-by: Nishanth Menon <nm@ti.com>
> > Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
> > Signed-off-by: Jai Luthra <j-luthra@ti.com>
> 
> Reviewed-by: Helen Koike <helen.koike@collabora.com>


Occurs on today's master: v6.5-8894-gb97d64c72259
https://gist.github.com/nmenon/6c7166171729342ee0be7de90b65c5c6#file-v6-5-8894-gb97d64c72259-L821

My only complaint with the patch is - yes, it does'nt crash, but I see
this spam on my console:
https://gist.github.com/nmenon/6c7166171729342ee0be7de90b65c5c6#file-with-patch-on-top-L236


> 
> > ---
> >  drivers/gpu/drm/bridge/ite-it66121.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> > index 466641c77fe9..d6fa00dea464 100644
> > --- a/drivers/gpu/drm/bridge/ite-it66121.c
> > +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> > @@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev, void *data,
> >  {
> >  	struct it66121_ctx *ctx = dev_get_drvdata(dev);
> >  
> > +	if (!ctx->connector) {
> > +		dev_dbg(dev, "No connector present, cannot provide EDID data");
> > +		return -EINVAL;
> > +	}
> > +
> >  	mutex_lock(&ctx->lock);
> >  
> >  	memcpy(buf, ctx->connector->eld,
> > 
> > ---
> > base-commit: 6269320850097903b30be8f07a5c61d9f7592393
> > change-id: 20230825-it66121_edid-6ee98517808b
> > 
> > Best regards,
> > -- 
> > Jai Luthra <j-luthra@ti.com>
> >
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm: bridge: it66121: Fix invalid connector dereference

[Jai Luthra]

[-- Attachment #1.1: Type: text/plain, Size: 2843 bytes --]

Hi Nishanth, Helen,

Thanks for the review.

On Aug 31, 2023 at 07:25:31 -0500, Nishanth Menon wrote:
> On 16:35-20230828, Helen Mae Koike Fornazier wrote:
> > On Friday, August 25, 2023 08:02 -03, Jai Luthra <j-luthra@ti.com> wrote:
> > 
> > > Fix the NULL pointer dereference when no monitor is connected, and the
> > > sound card is opened from userspace.
> > > 
> > > Instead return an error as EDID information cannot be provided to
> > > the sound framework if there is no connector attached.
> > > 
> > > Fixes: e0fd83dbe924 ("drm: bridge: it66121: Add audio support")
> > > Reported-by: Nishanth Menon <nm@ti.com>
> > > Closes: https://lore.kernel.org/all/20230825105849.crhon42qndxqif4i@gondola/
> > > Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > 
> > Reviewed-by: Helen Koike <helen.koike@collabora.com>
> 
> 
> Occurs on today's master: v6.5-8894-gb97d64c72259
> https://gist.github.com/nmenon/6c7166171729342ee0be7de90b65c5c6#file-v6-5-8894-gb97d64c72259-L821
> 
> My only complaint with the patch is - yes, it does'nt crash, but I see
> this spam on my console:
> https://gist.github.com/nmenon/6c7166171729342ee0be7de90b65c5c6#file-with-patch-on-top-L236
> 

Aradhya suggested an alternative approach [1] used by some bridges, 
where we return a buffer of 0s instead of an error here.

That will fix the spam, but more importantly will also allow playback if 
the HDMI monitor is hot-plugged later (after probe). I will send a new 
revision of this patch that uses that approach.

[1] https://lore.kernel.org/dri-devel/d2deac24-d5ab-e1c4-81c5-4874c2f5ea07@ti.com/

> 
> > 
> > > ---
> > >  drivers/gpu/drm/bridge/ite-it66121.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> > > index 466641c77fe9..d6fa00dea464 100644
> > > --- a/drivers/gpu/drm/bridge/ite-it66121.c
> > > +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> > > @@ -1446,6 +1446,11 @@ static int it66121_audio_get_eld(struct device *dev, void *data,
> > >  {
> > >  	struct it66121_ctx *ctx = dev_get_drvdata(dev);
> > >  
> > > +	if (!ctx->connector) {
> > > +		dev_dbg(dev, "No connector present, cannot provide EDID data");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	mutex_lock(&ctx->lock);
> > >  
> > >  	memcpy(buf, ctx->connector->eld,
> > > 
> > > ---
> > > base-commit: 6269320850097903b30be8f07a5c61d9f7592393
> > > change-id: 20230825-it66121_edid-6ee98517808b
> > > 
> > > Best regards,
> > > -- 
> > > Jai Luthra <j-luthra@ti.com>
> > >
> > 
> 
> -- 
> Regards,
> Nishanth Menon
> Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

-- 
Thanks,
Jai

GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel