| Message ID | I1AvHeOKt0LxZk5YfvzAd_iDpe5NWuhHNYueQ3Ubp3ilF-69Q8u5Nn_I-mqJt2zPKq1aoy2UsgT5hFUM1KMKVj8qXQHPqlvuVSulRLe1EAI=@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fix out-of-bounds access with short VSDB blocks | expand |
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
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, "