Re: [PATCH] netfilter: nf_conntrack_h323: fix NULL deref in decode_seqof()

[Florian Westphal <fw@strlen.de>]

Guanglei Zhu <zhugl3@xiaopeng.com> wrote:
> decode_seqof() loads son from f->fields and dereferences son->attr
> in the item loop without a NULL check.  A number of SEQOF entries
> in nf_conntrack_h323_types.c carry a NULL fields pointer, so a
> crafted packet that makes one of these optional fields present with
> a nonzero item count dereferences NULL in the loop.
> 
> Skip the decoding when fields is NULL.
> 
> Signed-off-by: Guanglei Zhu <zhugl3@xiaopeng.com>
> ---
> Verified on a KASAN kernel with a test module calling decode_seqof()
> with a fields=NULL SEQOF entry and a nonzero item count: NULL deref
> report at son->attr (read of size 2 at address 0x4).

We don't add NULL tests unless there is a sensibe reason.

Will you add NULL tests to other functions too because a 3rd party
module would crash otherwise?

If this is reachable, show how.

> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 6830c9da3..0d07d0276 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -691,6 +691,8 @@ static int decode_seqof(struct bitstr *bs, const struct field_t *f,
>  	}
>  
>  	/* Decode nested field */
> +	if (!f->fields)
> +		return H323_ERROR_NONE;
>  	son = f->fields;

This is strange.

son = f->fields;
if (!son)...

would be more natural.