| Message ID | C9C6655E-4974-4C04-8CEE-CA16563F4CC5@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, Aug 7, 2013 at 1:15 AM, James Bates <james.h.bates@gmail.com> wrote: > Hi all, > > 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. > > Signed-off-by: James Bates <james.h.bates@gmail.com> (CC'ing hpa) Yepp, this patch looks good: Reviewed-by: David Herrmann <dh.herrmann@gmail.com> 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 > --- > drivers/video/efifb.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c > index 50fe668..52d1d88 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 */ > -- > 1.7.12.4 (Apple Git-37) > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Fri, Aug 16, 2013 at 10:19 PM, James Bates <james.h.bates@gmail.com> 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@gmail.com> > > 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): You can put such comments below the "---" in your patch so they will not show up in the commit-message (between commit message and the diffstat). > 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@yahoo.com> > --- > 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 && From a quick glance, M_MBA_3 has "base == 0" but is a valid entry. Hm, I think we can be sure that there will never be a framebuffer at phys 0, so yeah, I am fine with this. Reviewed-by: David Herrmann <dh.herrmann@gmail.com> Regards David > + !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; > -- > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com <mailto:dh.herrmann@gmail.com>> >> >> 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@yahoo.com > <mailto:james.h.bates@yahoo.com>> > --- > 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; > -- >
diff --git a/drivers/video/efifb.c b/drivers/video/efifb.c index 50fe668..52d1d88 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 */
Hi all, 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. Signed-off-by: James Bates <james.h.bates@gmail.com> --- drivers/video/efifb.c | 3 --- 1 file changed, 3 deletions(-) -- 1.7.12.4 (Apple Git-37)