Possible Null pointer dereferences in net/mac80211/parse.c

Commit Message

Brahmajit March 10, 2025, 2:16 p.m. UTC

Coverity Scan reports that there might be a possible NULL pointer
dereferences in net/mac80211/parse.c: 1061 in
ieee802_11_parse_elems_full(). I understand that these reports are not
always correct.

I'm not sure whether the syntax
struct ieee80211_elems_parse_params sub = {};
is correct or falls under C11 standard[0].

initializer:
         assignment-expression
         { initializer-list }
         { initializer-list , }
initializer-list:
         designation(opt) initializer
         initializer-list , designation(opt) initializer

I'm aware that C23 allows empty initialization[1].

braced-initializer:
                    { }
                    { initializer-list }
                    { initializer-list , }

Considering [0], if we do something like


Would it be incorrect? Would appreciate some feedback.

[0]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
[1]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf

Comments

Jeff Johnson March 10, 2025, 5:09 p.m. UTC | #1

On 3/10/2025 7:16 AM, Brahmajit wrote:
> Coverity Scan reports that there might be a possible NULL pointer
> dereferences in net/mac80211/parse.c: 1061 in
> ieee802_11_parse_elems_full(). I understand that these reports are not
> always correct.
> 
> I'm not sure whether the syntax
> struct ieee80211_elems_parse_params sub = {};
> is correct or falls under C11 standard[0].

{} initializers are extensions supported by both gcc and clang/LLVM which
AFAIK are the only two compilers currently used by Linux.

> 
> initializer:
>          assignment-expression
>          { initializer-list }
>          { initializer-list , }
> initializer-list:
>          designation(opt) initializer
>          initializer-list , designation(opt) initializer
> 
> I'm aware that C23 allows empty initialization[1].
> 
> braced-initializer:
>                     { }
>                     { initializer-list }
>                     { initializer-list , }
> 
> Considering [0], if we do something like
> 
> --- a/net/mac80211/parse.c
> +++ b/net/mac80211/parse.c
> @@ -997,7 +997,7 @@ ieee80211_mle_defrag_epcs(struct ieee80211_elems_parse *elems_parse)
>  struct ieee802_11_elems *
>  ieee802_11_parse_elems_full(struct ieee80211_elems_parse_params *params)
>  {
> -       struct ieee80211_elems_parse_params sub = {};
> +       struct ieee80211_elems_parse_params sub = { 0 };

using one of the supported compilers, these are identical for this struct.

note that {} is usually preferable to {0} since {} works even when the first
member is not a scalar.

and the coverity report indicates the issue is that sub.start can be NULL,
which will initially be true with either of these initializers.

so the question is can sub.start really be NULL at the point where it is
passed to cfg80211_find_elem(), or does every path initialize it?

that is the question you should try to answer

>         struct ieee80211_elems_parse *elems_parse;
>         const struct element *non_inherit = NULL;
>         struct ieee802_11_elems *elems;
> 
> Would it be incorrect? Would appreciate some feedback.
> 
> [0]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
> [1]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf

Patch

--- a/net/mac80211/parse.c
+++ b/net/mac80211/parse.c
@@ -997,7 +997,7 @@  ieee80211_mle_defrag_epcs(struct ieee80211_elems_parse *elems_parse)
 struct ieee802_11_elems *
 ieee802_11_parse_elems_full(struct ieee80211_elems_parse_params *params)
 {
-       struct ieee80211_elems_parse_params sub = {};
+       struct ieee80211_elems_parse_params sub = { 0 };
        struct ieee80211_elems_parse *elems_parse;
        const struct element *non_inherit = NULL;
        struct ieee802_11_elems *elems;