| Message ID | 20210920141101.194959-1-lma@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/i915/bdb: Fix version check | expand |
On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote: > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+" > the size of bdb_lfp_backlight_data structure has been increased, > causing if-statement in the parse_lfp_backlight function > that comapres this structure size to the one retrieved from BDB, > always to fail for older revisions. > This patch fixes it by comparing a total size of all fileds from > the structure (present before the change) with the value gathered from BDB. > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221) > > Cc: <stable@vger.kernel.org> # 5.4+ > Tested-by: Lukasz Majczak <lma@semihalf.com> > Signed-off-by: Lukasz Majczak <lma@semihalf.com> > --- > drivers/gpu/drm/i915/display/intel_bios.c | 4 +++- > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > index 3c25926092de..052a19b455d1 100644 > --- a/drivers/gpu/drm/i915/display/intel_bios.c > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915, > > i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI; > if (bdb->version >= 191 && > - get_blocksize(backlight_data) >= sizeof(*backlight_data)) { > + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) + > + sizeof(backlight_data->data) + > + sizeof(backlight_data->level))) { Missing sizeof(backlight_data->backlight_control) but this is getting very verbose. Would be better have a expected size variable set each version set in the beginning of this function. something like: switch (bdb->version) { case 191: expected_size = x; break; case 234: expected_size = x; break; case 236: default: expected_size = x; } > const struct lfp_backlight_control_method *method; > > method = &backlight_data->backlight_control[panel_type]; > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > index 330077c2e588..fff456bf8783 100644 > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > @@ -814,6 +814,11 @@ struct lfp_brightness_level { > u16 reserved; > } __packed; > > +/* > + * Changing struct bdb_lfp_backlight_data might affect its > + * size comparation to the value hold in BDB. > + * (e.g. in parse_lfp_backlight()) > + */ This is true for all the blocks so I don't think we need this comment. > struct bdb_lfp_backlight_data { > u8 entry_size; > struct lfp_backlight_data_entry data[16];
pon., 20 wrz 2021 o 22:47 Souza, Jose <jose.souza@intel.com> napisał(a): > > On Mon, 2021-09-20 at 16:11 +0200, Lukasz Majczak wrote: > > With patch "drm/i915/vbt: Fix backlight parsing for VBT 234+" > > the size of bdb_lfp_backlight_data structure has been increased, > > causing if-statement in the parse_lfp_backlight function > > that comapres this structure size to the one retrieved from BDB, > > always to fail for older revisions. > > This patch fixes it by comparing a total size of all fileds from > > the structure (present before the change) with the value gathered from BDB. > > Tested on Chromebook Pixelbook (Nocturne) (reports bdb->version = 221) > > > > Cc: <stable@vger.kernel.org> # 5.4+ > > Tested-by: Lukasz Majczak <lma@semihalf.com> > > Signed-off-by: Lukasz Majczak <lma@semihalf.com> > > --- > > drivers/gpu/drm/i915/display/intel_bios.c | 4 +++- > > drivers/gpu/drm/i915/display/intel_vbt_defs.h | 5 +++++ > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c > > index 3c25926092de..052a19b455d1 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bios.c > > +++ b/drivers/gpu/drm/i915/display/intel_bios.c > > @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915, > > > > i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI; > > if (bdb->version >= 191 && > > - get_blocksize(backlight_data) >= sizeof(*backlight_data)) { > > + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) + > > + sizeof(backlight_data->data) + > > + sizeof(backlight_data->level))) { > > Missing sizeof(backlight_data->backlight_control) but this is getting very verbose. > Would be better have a expected size variable set each version set in the beginning of this function. > > something like: > switch (bdb->version) { > case 191: > expected_size = x; > break; > case 234: > expected_size = x; > break; > case 236: > default: > expected_size = x; > } > > > > const struct lfp_backlight_control_method *method; > > > > method = &backlight_data->backlight_control[panel_type]; > > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > index 330077c2e588..fff456bf8783 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > @@ -814,6 +814,11 @@ struct lfp_brightness_level { > > u16 reserved; > > } __packed; > > > > +/* > > + * Changing struct bdb_lfp_backlight_data might affect its > > + * size comparation to the value hold in BDB. > > + * (e.g. in parse_lfp_backlight()) > > + */ > > This is true for all the blocks so I don't think we need this comment. > > > struct bdb_lfp_backlight_data { > > u8 entry_size; > > struct lfp_backlight_data_entry data[16]; > Hi Jose, Jani Jani - you are right - I was working on 5.4 with a backported patch - I'm sorry for this confusion. Jose, Regarding expected_size, I couldn't find documentation that could described this structure size changes among revisions, so all I could do is to do an educated guess, basing on comments at this structure, like: (gdb) ptype /o struct bdb_lfp_backlight_data /* offset | size */ type = struct bdb_lfp_backlight_data { /* 0 | 1 */ u8 entry_size; /* 1 | 96 */ struct lfp_backlight_data_entry data[16]; /* 97 | 16 */ u8 level[16]; /* 113 | 16 */ struct lfp_backlight_control_method backlight_control[16]; /* 129 | 64 */ struct lfp_brightness_level brightness_level[16]; /* 234+ */ /* 193 | 64 */ struct lfp_brightness_level brightness_min_level[16]; /* 234+ */ /* 257 | 16 */ u8 brightness_precision_bits[16]; /* 236+ */ /* total size (bytes): 273 */ } if (revision <= 234) expected_size = 129; else if (revision > 234 && revision <=236) expected_size = 257; else /* revision > 236 */ expected_size = 273; Is this approach ok? Otherwise I think I would need help from you to get exact numbers for each revision... Best regards, Lukasz
- dropping stable ... > > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > index 330077c2e588..fff456bf8783 100644 > > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h > > @@ -814,6 +814,11 @@ struct lfp_brightness_level { > > u16 reserved; > > } __packed; > > > > +/* > > + * Changing struct bdb_lfp_backlight_data might affect its > > + * size comparation to the value hold in BDB. > > + * (e.g. in parse_lfp_backlight()) > > + */ > > This is true for all the blocks so I don't think we need this comment. Lack of such comment was probable cause of this overlook. As this is an example of the consequence (bricking platforms dependent on mentioned conditions) IMO we need some comment here, or this will probably happen again. > > > struct bdb_lfp_backlight_data { > > u8 entry_size; > > struct lfp_backlight_data_entry data[16]; >
On Tue, 21 Sep 2021, Radosław Biernacki <rad@semihalf.com> wrote: > - dropping stable > > ... > >> > diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h >> > index 330077c2e588..fff456bf8783 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h >> > +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h >> > @@ -814,6 +814,11 @@ struct lfp_brightness_level { >> > u16 reserved; >> > } __packed; >> > >> > +/* >> > + * Changing struct bdb_lfp_backlight_data might affect its >> > + * size comparation to the value hold in BDB. >> > + * (e.g. in parse_lfp_backlight()) >> > + */ >> >> This is true for all the blocks so I don't think we need this comment. > > Lack of such comment was probable cause of this overlook. > As this is an example of the consequence (bricking platforms dependent > on mentioned conditions) IMO we need some comment here, or this will > probably happen again. The whole file is full of __packed structs with the sole purpose of parsing VBT data in memory. People are generally well aware of the consequences of changing the size, and this is the only such mistake I can recall. BR, Jani. > > >> >> > struct bdb_lfp_backlight_data { >> > u8 entry_size; >> > struct lfp_backlight_data_entry data[16]; >>
diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c index 3c25926092de..052a19b455d1 100644 --- a/drivers/gpu/drm/i915/display/intel_bios.c +++ b/drivers/gpu/drm/i915/display/intel_bios.c @@ -452,7 +452,9 @@ parse_lfp_backlight(struct drm_i915_private *i915, i915->vbt.backlight.type = INTEL_BACKLIGHT_DISPLAY_DDI; if (bdb->version >= 191 && - get_blocksize(backlight_data) >= sizeof(*backlight_data)) { + get_blocksize(backlight_data) >= (sizeof(backlight_data->entry_size) + + sizeof(backlight_data->data) + + sizeof(backlight_data->level))) { const struct lfp_backlight_control_method *method; method = &backlight_data->backlight_control[panel_type]; diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h index 330077c2e588..fff456bf8783 100644 --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h @@ -814,6 +814,11 @@ struct lfp_brightness_level { u16 reserved; } __packed; +/* + * Changing struct bdb_lfp_backlight_data might affect its + * size comparation to the value hold in BDB. + * (e.g. in parse_lfp_backlight()) + */ struct bdb_lfp_backlight_data { u8 entry_size; struct lfp_backlight_data_entry data[16];