Re: [PATCH] efifb: prevent null dereferences by removing unused arrayindices from dmi_list

[David Herrmann]

Hi

On Wed, Aug 7, 2013 at 1:15 AM, James Bates <james.h.bates@xxxxxxxxx> 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@xxxxxxxxx>

(CC'ing hpa)

Yepp, this patch looks good:
Reviewed-by: David Herrmann <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

> ---
>  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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/