On Thu, 04 Apr 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> EDID read functions do not validate their return data. So they might
> return the required number of bytes of probing, but with invalid
> data. Therefore test for the presence of an EDID header similar to
> the code in edid_block_check().
I don't think the point of drm_probe_ddc() ever was to validate
anything. It reads one byte to see if there's any response. That's all
there is to it.
All EDID validation happens in the _drm_do_get_edid() when actually
reading the EDID.
I don't think I like duplicating this behaviour in both the probe and
the EDID read. And I'm not sure why we're giving drivers the option to
pass a parameter whether to validate or not. Just why?
BR,
Jani.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> drivers/gpu/drm/drm_edid.c | 50 +++++++++++++++++++++++++++++---------
> include/drm/drm_edid.h | 2 +-
> 2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a3e9333f9177a..4e12d4b83a720 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1758,6 +1758,18 @@ static void edid_header_fix(void *edid)
> memcpy(edid, edid_header, sizeof(edid_header));
> }
>
> +static int edid_header_score(const u8 *header)
> +{
> + int i, score = 0;
> +
> + for (i = 0; i < sizeof(edid_header); i++) {
> + if (header[i] == edid_header[i])
> + score++;
> + }
> +
> + return score;
> +}
> +
> /**
> * drm_edid_header_is_valid - sanity check the header of the base EDID block
> * @_edid: pointer to raw base EDID block
> @@ -1769,14 +1781,8 @@ static void edid_header_fix(void *edid)
> int drm_edid_header_is_valid(const void *_edid)
> {
> const struct edid *edid = _edid;
> - int i, score = 0;
>
> - for (i = 0; i < sizeof(edid_header); i++) {
> - if (edid->header[i] == edid_header[i])
> - score++;
> - }
> -
> - return score;
> + return edid_header_score(edid->header);
> }
> EXPORT_SYMBOL(drm_edid_header_is_valid);
>
> @@ -2612,17 +2618,37 @@ EXPORT_SYMBOL(drm_edid_free);
> * drm_edid_probe_custom() - probe DDC presence
> * @read_block: EDID block read function
> * @context: Private data passed to the block read function
> + * @validate: True to validate the EDID header
> *
> * Probes for EDID data. Only reads enough data to detect the presence
> - * of the EDID channel.
> + * of the EDID channel. Some EDID block read functions do not fail,
> + * but return invalid data if no EDID data is available. If @validate
> + * has been specified, drm_edid_probe_custom() validates the retrieved
> + * EDID header before signalling success.
> *
> * Return: True on success, false on failure.
> */
> -bool drm_edid_probe_custom(read_block_fn read_block, void *context)
> +bool drm_edid_probe_custom(read_block_fn read_block, void *context, bool validate)
> {
> - unsigned char out;
> + u8 header[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> + int ret;
> + size_t len = 1;
> +
> + if (validate)
> + len = sizeof(header); // read full header
> +
> + ret = read_block(context, header, 0, len);
> + if (ret)
> + return false;
> +
> + if (validate) {
> + int score = edid_header_score(header);
> +
> + if (score < clamp(edid_fixup, 0, 8))
> + return false;
> + }
>
> - return (read_block(context, &out, 0, 1) == 0);
> + return true;
> }
> EXPORT_SYMBOL(drm_edid_probe_custom);
>
> @@ -2635,7 +2661,7 @@ EXPORT_SYMBOL(drm_edid_probe_custom);
> bool
> drm_probe_ddc(struct i2c_adapter *adapter)
> {
> - return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter);
> + return drm_edid_probe_custom(drm_do_probe_ddc_edid, adapter, false);
> }
> EXPORT_SYMBOL(drm_probe_ddc);
>
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 4d1797ade5f1d..299278c7ee1c2 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -412,7 +412,7 @@ static inline void drm_edid_decode_panel_id(u32 panel_id, char vend[4], u16 *pro
>
> bool
> drm_edid_probe_custom(int (*read_block)(void *context, u8 *buf, unsigned int block, size_t len),
> - void *context);
> + void *context, bool validate);
> bool drm_probe_ddc(struct i2c_adapter *adapter);
> struct edid *drm_do_get_edid(struct drm_connector *connector,
> int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
--
Jani Nikula, Intel
