On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote:
> nft list table bridge t
> table bridge t {
> set s4 {
> typeof ip saddr . ip daddr
> elements = { 1.0.0.1 . 2.0.0.2 }
> }
> }
> =================================================================
> ==24334==ERROR: AddressSanitizer: heap-use-after-free on address 0x6080000000a8 at pc 0x7fe0e67df0ad bp 0x7ffff83e88c0 sp 0x7ffff83e88b8
> READ of size 4 at 0x6080000000a8 thread T0
> #0 0x7fe0e67df0ac in datatype_free nftables/src/datatype.c:1110
> #1 0x7fe0e67e2092 in expr_free nftables/src/expression.c:89
> #2 0x7fe0e67a855e in set_free nftables/src/rule.c:359
> #3 0x7fe0e67b2f3e in table_free nftables/src/rule.c:1263
> #4 0x7fe0e67a70ce in __cache_flush nftables/src/rule.c:299
> #5 0x7fe0e67a71c7 in cache_release nftables/src/rule.c:305
> #6 0x7fe0e68dbfa9 in nft_ctx_free nftables/src/libnftables.c:292
> #7 0x55f00fbe0051 in main nftables/src/main.c:469
> #8 0x7fe0e553309a in __libc_start_main ../csu/libc-start.c:308
> #9 0x55f00fbdd429 in _start (nftables/src/.libs/nft+0x9429)
>
> 0x6080000000a8 is located 8 bytes inside of 96-byte region [0x6080000000a0,0x608000000100)
> freed by thread T0 here:
> #0 0x7fe0e6e70fb0 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe8fb0)
> #1 0x7fe0e68b8122 in xfree nftables/src/utils.c:29
> #2 0x7fe0e67df2e5 in datatype_free nftables/src/datatype.c:1117
> #3 0x7fe0e67e2092 in expr_free nftables/src/expression.c:89
> #4 0x7fe0e67a83fe in set_free nftables/src/rule.c:356
> #5 0x7fe0e67b2f3e in table_free nftables/src/rule.c:1263
> #6 0x7fe0e67a70ce in __cache_flush nftables/src/rule.c:299
> #7 0x7fe0e67a71c7 in cache_release nftables/src/rule.c:305
> #8 0x7fe0e68dbfa9 in nft_ctx_free nftables/src/libnftables.c:292
> #9 0x55f00fbe0051 in main nftables/src/main.c:469
> #10 0x7fe0e553309a in __libc_start_main ../csu/libc-start.c:308
>
> previously allocated by thread T0 here:
> #0 0x7fe0e6e71330 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe9330)
> #1 0x7fe0e68b813d in xmalloc nftables/src/utils.c:36
> #2 0x7fe0e68b8296 in xzalloc nftables/src/utils.c:65
> #3 0x7fe0e67de7d5 in dtype_alloc nftables/src/datatype.c:1065
> #4 0x7fe0e67df862 in concat_type_alloc nftables/src/datatype.c:1146
> #5 0x7fe0e67ea852 in concat_expr_parse_udata nftables/src/expression.c:954
> #6 0x7fe0e685dc94 in set_make_key nftables/src/netlink.c:718
> #7 0x7fe0e685e177 in netlink_delinearize_set nftables/src/netlink.c:770
> #8 0x7fe0e685f667 in list_set_cb nftables/src/netlink.c:895
> #9 0x7fe0e4f95a03 in nftnl_set_list_foreach src/set.c:904
>
> SUMMARY: AddressSanitizer: heap-use-after-free nftables/src/datatype.c:1110 in datatype_free
> Shadow bytes around the buggy address:
> 0x0c107fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c107fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c107fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c107fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c107fff8000: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
> =>0x0c107fff8010: fa fa fa fa fd[fd]fd fd fd fd fd fd fd fd fd fd
> 0x0c107fff8020: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c107fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c107fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c107fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c107fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable: 00
> Partially addressable: 01 02 03 04 05 06 07
> Heap left redzone: fa
> Freed heap region: fd
> Stack left redzone: f1
> Stack mid redzone: f2
> Stack right redzone: f3
> Stack after return: f5
> Stack use after scope: f8
> Global redzone: f9
> Global init order: f6
> Poisoned by user: f7
> Container overflow: fc
> Array cookie: ac
> Intra object redzone: bb
> ASan internal: fe
> Left alloca redzone: ca
> Right alloca redzone: cb
> ==24334==ABORTING
> ---
> src/datatype.c | 2 ++
> src/expression.c | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/datatype.c b/src/datatype.c
> index 095598d9..0110846f 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -1083,6 +1083,8 @@ struct datatype *datatype_get(const struct datatype *ptr)
>
> void datatype_set(struct expr *expr, const struct datatype *dtype)
> {
> + if (dtype == expr->dtype)
> + return; // do not free dtype before incrementing refcnt again
This makes sense indeed. If the same dtype is set, then turning this
to noop is fine.
The problem you describe (use-after-free) happens in this case, right?
datatype_set(expr, expr->dtype);
Or am I missing anything?
> datatype_free(expr->dtype);
> expr->dtype = datatype_get(dtype);
> }
> diff --git a/src/expression.c b/src/expression.c
> index 6605beb3..a6bde70f 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -955,7 +955,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
> if (!dtype)
> goto err_free;
>
> - concat_expr->dtype = dtype;
> + concat_expr->dtype = datatype_get(dtype);
This is also good indeed.
Thanks.
