Hi James, Can you resend this? I don't know how I could apply the patch from this mail without re-creating it manually. Tomi On 16/08/13 23:19, James Bates wrote: > On Fri, 2013-08-16 at 15:37 +0200, David Herrmann wrote: >> Hi >> (CC'ing hpa) >> >> Yepp, this patch looks good: >> Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxx <mailto:dh.herrmann@xxxxxxxxx>> >> >> However, I'd like to see some code to prevent this from happening >> again. It isn't really obvious that removing an entry will result in a >> NULL-deref. I am not the maintainer of this code, but I'd really like >> to see a "(dmi_list[i].optname && !strcmp())" check in efifb_setup(). >> Otherwise we _will_ run into this again. >> >> Side note: this code got moved to arch/x86/kernel/sysfb_efi.c in the >> x86 tree. I am adding hpa here so he will remember this once Linus >> gets a merge conflict (iff the sysfb changes get merged through the >> x86 tree). >> >> Thanks >> David > Sure thing, here is the patch again, with the extra check in > efifb_setup() (it turns out one can simply > switch around the order of the checks: implicitly initialized dmi_list > items will have base == 0): > > Full patch v2: > the dmi_list array is initialized using gnu designated initializers, and > therefore > contains fewer explicitly defined entries as there are elements in it. This > is because the enum above with M_blabla constants contains more items > than the designated > initializer. Those elements not explicitly initialized are implicitly > set to 0. > > Now efifb_setup(), L.322 & L.323, loops through all these array > elements, and performs > a strcmp o a field (optname) in each item. For non explicitly > initialized elements this > will be a null pointer: > > for (i = 0; i < M_UNKNOWN; i++) { > if (!strcmp(this_opt, > dmi_list[i].optname) && > > On my macbook6,1 the predefined values are for some reason incorrect, > and most parameters > are preset correctly by my efi bootloader (elilo). but > stride/line_length is not detected > correctly and so I wish to set it explicitly using a > "video=efifb:stride:2048" command-line > argument. Because of the above null dereference, an exception > (presumably) occurs before > the parsing code (L.333) is ever reached. I say presumably since the mac > hangs on boot > without a console, and I can therefore not see any output. > > By removing the unused values from the enum, and thus preventing > implicitly initialized items > in the dmi_list array, the null dereference does not occur, my customer > command-line arg is > parsed correctly, and my console displays correctly. > > This patch removes the unused enum values, and also guards against any > future implicit > initializing by inverting the check order in the if statement, and > checking first whether > dmi_list[i].base is null. > > Signed-off-by: James Bates <james.h.bates@xxxxxxxxx > <mailto:james.h.bates@xxxxxxxxx>> > --- > drivers/video/efifb.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c > index 50fe668..161757b 100644 > --- a/drivers/video/efifb.c > +++ b/drivers/video/efifb.c > @@ -50,12 +50,9 @@ enum { > M_MINI_3_1, /* Mac Mini, 3,1th gen */ > M_MINI_4_1, /* Mac Mini, 4,1th gen */ > M_MB, /* MacBook */ > - M_MB_2, /* MacBook, 2nd rev. */ > - M_MB_3, /* MacBook, 3rd rev. */ > M_MB_5_1, /* MacBook, 5th rev. */ > M_MB_6_1, /* MacBook, 6th rev. */ > M_MB_7_1, /* MacBook, 7th rev. */ > - M_MB_SR, /* MacBook, 2nd gen, (Santa Rosa) */ > M_MBA, /* MacBook Air */ > M_MBA_3, /* Macbook Air, 3rd rev */ > M_MBP, /* MacBook Pro */ > @@ -323,8 +320,8 @@ static int __init efifb_setup(char *options) > if (!*this_opt) continue; > > for (i = 0; i < M_UNKNOWN; i++) { > - if (!strcmp(this_opt, dmi_list[i].optname) && > - dmi_list[i].base != 0) { > + if (dmi_list[i].base != 0 && > + !strcmp(this_opt, dmi_list[i].optname)) { > screen_info.lfb_base = dmi_list[i].base; > screen_info.lfb_linelength = dmi_list[i].stride; > screen_info.lfb_width = dmi_list[i].width; > -- >
Attachment:
signature.asc
Description: OpenPGP digital signature
