drm: fix out-of-bounds access with short VSDB blocks

Submitted by Simon Ser on July 22, 2019, 2:38 p.m.

Details

Message ID I1AvHeOKt0LxZk5YfvzAd_iDpe5NWuhHNYueQ3Ubp3ilF-69Q8u5Nn_I-mqJt2zPKq1aoy2UsgT5hFUM1KMKVj8qXQHPqlvuVSulRLe1EAI=@emersion.fr
State New
Headers show
Series "drm: fix out-of-bounds access with short VSDB blocks" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Simon Ser July 22, 2019, 2:38 p.m.
From: Simon Ser <simon.ser@intel.com>

The VSDB parsing code contains a few len >= N checks, accessing db[N] on
success. However if len == N, db[N] is out-of-bounds.

This commit changes the checks to test for len > N.

Signed-off-by: Simon Ser <contact@emersion.fr>
---
 drivers/gpu/drm/drm_edid.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

--
2.22.0

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 82a4ceed3fcf..13d632f14172 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3569,7 +3569,7 @@  do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
 	vic_len = db[8 + offset] >> 5;
 	hdmi_3d_len = db[8 + offset] & 0x1f;

-	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
+	for (i = 0; i < vic_len && len > (9 + offset + i); i++) {
 		u8 vic;

 		vic = db[9 + offset + i];
@@ -3971,11 +3971,11 @@  drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)
 	connector->hdr_sink_metadata.hdmi_type1.metadata_type =
 						hdr_metadata_type(db);

-	if (len >= 4)
+	if (len > 4)
 		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
-	if (len >= 5)
+	if (len > 5)
 		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
-	if (len >= 6)
+	if (len > 6)
 		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
 }

@@ -3984,19 +3984,19 @@  drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 {
 	u8 len = cea_db_payload_len(db);

-	if (len >= 6 && (db[6] & (1 << 7)))
+	if (len > 6 && (db[6] & (1 << 7)))
 		connector->eld[DRM_ELD_SAD_COUNT_CONN_TYPE] |= DRM_ELD_SUPPORTS_AI;
-	if (len >= 8) {
+	if (len > 8) {
 		connector->latency_present[0] = db[8] >> 7;
 		connector->latency_present[1] = (db[8] >> 6) & 1;
 	}
-	if (len >= 9)
+	if (len > 9)
 		connector->video_latency[0] = db[9];
-	if (len >= 10)
+	if (len > 10)
 		connector->audio_latency[0] = db[10];
-	if (len >= 11)
+	if (len > 11)
 		connector->video_latency[1] = db[11];
-	if (len >= 12)
+	if (len > 12)
 		connector->audio_latency[1] = db[12];

 	DRM_DEBUG_KMS("HDMI: latency present %d %d, "
@@ -4559,9 +4559,9 @@  drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
 	struct drm_display_info *info = &connector->display_info;
 	u8 len = cea_db_payload_len(db);

-	if (len >= 6)
+	if (len > 6)
 		info->dvi_dual = db[6] & 1;
-	if (len >= 7)
+	if (len > 7)
 		info->max_tmds_clock = db[7] * 5000;

 	DRM_DEBUG_KMS("HDMI: DVI dual %d, "

Comments

On Mon, 2019-07-22 at 14:38 +0000, Simon Ser wrote:
> From: Simon Ser <simon.ser@intel.com>

> 

> The VSDB parsing code contains a few len >= N checks, accessing db[N] on

> success. However if len == N, db[N] is out-of-bounds.

> 

> This commit changes the checks to test for len > N.

> 

> Signed-off-by: Simon Ser <contact@emersion.fr>


Bleh, I messed up the S-o-b.

Signed-off-by: Simon Ser <simon.ser@intel.com>


Sorry about that.

> ---

>  drivers/gpu/drm/drm_edid.c | 24 ++++++++++++------------

>  1 file changed, 12 insertions(+), 12 deletions(-)

> 

> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c

> index 82a4ceed3fcf..13d632f14172 100644

> --- a/drivers/gpu/drm/drm_edid.c

> +++ b/drivers/gpu/drm/drm_edid.c

> @@ -3569,7 +3569,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,

>  	vic_len = db[8 + offset] >> 5;

>  	hdmi_3d_len = db[8 + offset] & 0x1f;

> 

> -	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {

> +	for (i = 0; i < vic_len && len > (9 + offset + i); i++) {

>  		u8 vic;

> 

>  		vic = db[9 + offset + i];

> @@ -3971,11 +3971,11 @@ drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)

>  	connector->hdr_sink_metadata.hdmi_type1.metadata_type =

>  						hdr_metadata_type(db);

> 

> -	if (len >= 4)

> +	if (len > 4)

>  		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];

> -	if (len >= 5)

> +	if (len > 5)

>  		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];

> -	if (len >= 6)

> +	if (len > 6)

>  		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];

>  }

> 

> @@ -3984,19 +3984,19 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)

>  {

>  	u8 len = cea_db_payload_len(db);

> 

> -	if (len >= 6 && (db[6] & (1 << 7)))

> +	if (len > 6 && (db[6] & (1 << 7)))

>  		connector->eld[DRM_ELD_SAD_COUNT_CONN_TYPE] |= DRM_ELD_SUPPORTS_AI;

> -	if (len >= 8) {

> +	if (len > 8) {

>  		connector->latency_present[0] = db[8] >> 7;

>  		connector->latency_present[1] = (db[8] >> 6) & 1;

>  	}

> -	if (len >= 9)

> +	if (len > 9)

>  		connector->video_latency[0] = db[9];

> -	if (len >= 10)

> +	if (len > 10)

>  		connector->audio_latency[0] = db[10];

> -	if (len >= 11)

> +	if (len > 11)

>  		connector->video_latency[1] = db[11];

> -	if (len >= 12)

> +	if (len > 12)

>  		connector->audio_latency[1] = db[12];

> 

>  	DRM_DEBUG_KMS("HDMI: latency present %d %d, "

> @@ -4559,9 +4559,9 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)

>  	struct drm_display_info *info = &connector->display_info;

>  	u8 len = cea_db_payload_len(db);

> 

> -	if (len >= 6)

> +	if (len > 6)

>  		info->dvi_dual = db[6] & 1;

> -	if (len >= 7)

> +	if (len > 7)

>  		info->max_tmds_clock = db[7] * 5000;

> 

>  	DRM_DEBUG_KMS("HDMI: DVI dual %d, "

> --

> 2.22.0

> 

>
On 22/07/2019 15:38, Simon Ser wrote:
> From: Simon Ser <simon.ser@intel.com>
> 
> The VSDB parsing code contains a few len >= N checks, accessing db[N] on
> success. However if len == N, db[N] is out-of-bounds.

I'm not familiar with VSDB parsing, but there's a comment before the
function:

>  * @len: length of the CEA block payload, ie. one can access up to db[len]

i.e. length is one shorter than you might expect.

Digging through the history there a post from 2013[1] when the code was
being reviewed:

Ville Syrjälä wrote:
> We skip the 'tag/len' byte for do_cea_modes(), but here we include it.
> The resulting indexing is maybe a bit easier to compare w/ the spec,
> but one must always keep in mind that the payload length doesn't include
> byte 0. Not sure if we should just increase len by 1, or skip the tag
> byte here too. Or maybe just add a comment explaining the situation.

[1] https://lists.freedesktop.org/archives/intel-gfx/2013-August/031376.html

So it looks like that comment for 'len' was meant to explain the unusual
length. I have to admit that I didn't find the comment obvious, and the
code does look suspect though.

Did you actually see any problems in practice with the existing code?

Steve

> 
> This commit changes the checks to test for len > N.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>  drivers/gpu/drm/drm_edid.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 82a4ceed3fcf..13d632f14172 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3569,7 +3569,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  	vic_len = db[8 + offset] >> 5;
>  	hdmi_3d_len = db[8 + offset] & 0x1f;
> 
> -	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
> +	for (i = 0; i < vic_len && len > (9 + offset + i); i++) {
>  		u8 vic;
> 
>  		vic = db[9 + offset + i];
> @@ -3971,11 +3971,11 @@ drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)
>  	connector->hdr_sink_metadata.hdmi_type1.metadata_type =
>  						hdr_metadata_type(db);
> 
> -	if (len >= 4)
> +	if (len > 4)
>  		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
> -	if (len >= 5)
> +	if (len > 5)
>  		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
> -	if (len >= 6)
> +	if (len > 6)
>  		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
>  }
> 
> @@ -3984,19 +3984,19 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
>  {
>  	u8 len = cea_db_payload_len(db);
> 
> -	if (len >= 6 && (db[6] & (1 << 7)))
> +	if (len > 6 && (db[6] & (1 << 7)))
>  		connector->eld[DRM_ELD_SAD_COUNT_CONN_TYPE] |= DRM_ELD_SUPPORTS_AI;
> -	if (len >= 8) {
> +	if (len > 8) {
>  		connector->latency_present[0] = db[8] >> 7;
>  		connector->latency_present[1] = (db[8] >> 6) & 1;
>  	}
> -	if (len >= 9)
> +	if (len > 9)
>  		connector->video_latency[0] = db[9];
> -	if (len >= 10)
> +	if (len > 10)
>  		connector->audio_latency[0] = db[10];
> -	if (len >= 11)
> +	if (len > 11)
>  		connector->video_latency[1] = db[11];
> -	if (len >= 12)
> +	if (len > 12)
>  		connector->audio_latency[1] = db[12];
> 
>  	DRM_DEBUG_KMS("HDMI: latency present %d %d, "
> @@ -4559,9 +4559,9 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
>  	struct drm_display_info *info = &connector->display_info;
>  	u8 len = cea_db_payload_len(db);
> 
> -	if (len >= 6)
> +	if (len > 6)
>  		info->dvi_dual = db[6] & 1;
> -	if (len >= 7)
> +	if (len > 7)
>  		info->max_tmds_clock = db[7] * 5000;
> 
>  	DRM_DEBUG_KMS("HDMI: DVI dual %d, "
> --
> 2.22.0
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
On Mon, Jul 22, 2019 at 02:38:34PM +0000, Simon Ser wrote:
> From: Simon Ser <simon.ser@intel.com>
> 
> The VSDB parsing code contains a few len >= N checks, accessing db[N] on
> success. However if len == N, db[N] is out-of-bounds.
> 
> This commit changes the checks to test for len > N.

I'm not familiar with this at all, but is db[0] the header and db[1] to db[len]
the data block? In that case, it's not out of bounds.

I looked up the spec and it says:
         Length of following data block (in bytes) (02h)

Sean

> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>  drivers/gpu/drm/drm_edid.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 82a4ceed3fcf..13d632f14172 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3569,7 +3569,7 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
>  	vic_len = db[8 + offset] >> 5;
>  	hdmi_3d_len = db[8 + offset] & 0x1f;
> 
> -	for (i = 0; i < vic_len && len >= (9 + offset + i); i++) {
> +	for (i = 0; i < vic_len && len > (9 + offset + i); i++) {
>  		u8 vic;
> 
>  		vic = db[9 + offset + i];
> @@ -3971,11 +3971,11 @@ drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)
>  	connector->hdr_sink_metadata.hdmi_type1.metadata_type =
>  						hdr_metadata_type(db);
> 
> -	if (len >= 4)
> +	if (len > 4)
>  		connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
> -	if (len >= 5)
> +	if (len > 5)
>  		connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
> -	if (len >= 6)
> +	if (len > 6)
>  		connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
>  }
> 
> @@ -3984,19 +3984,19 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
>  {
>  	u8 len = cea_db_payload_len(db);
> 
> -	if (len >= 6 && (db[6] & (1 << 7)))
> +	if (len > 6 && (db[6] & (1 << 7)))
>  		connector->eld[DRM_ELD_SAD_COUNT_CONN_TYPE] |= DRM_ELD_SUPPORTS_AI;
> -	if (len >= 8) {
> +	if (len > 8) {
>  		connector->latency_present[0] = db[8] >> 7;
>  		connector->latency_present[1] = (db[8] >> 6) & 1;
>  	}
> -	if (len >= 9)
> +	if (len > 9)
>  		connector->video_latency[0] = db[9];
> -	if (len >= 10)
> +	if (len > 10)
>  		connector->audio_latency[0] = db[10];
> -	if (len >= 11)
> +	if (len > 11)
>  		connector->video_latency[1] = db[11];
> -	if (len >= 12)
> +	if (len > 12)
>  		connector->audio_latency[1] = db[12];
> 
>  	DRM_DEBUG_KMS("HDMI: latency present %d %d, "
> @@ -4559,9 +4559,9 @@ drm_parse_hdmi_vsdb_video(struct drm_connector *connector, const u8 *db)
>  	struct drm_display_info *info = &connector->display_info;
>  	u8 len = cea_db_payload_len(db);
> 
> -	if (len >= 6)
> +	if (len > 6)
>  		info->dvi_dual = db[6] & 1;
> -	if (len >= 7)
> +	if (len > 7)
>  		info->max_tmds_clock = db[7] * 5000;
> 
>  	DRM_DEBUG_KMS("HDMI: DVI dual %d, "
> --
> 2.22.0
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel