| Message ID | 20200501154819.2984-3-michael-dev@fami-braun.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [1/3] main: fix ASAN -fsanizize=address error | expand |
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.
Am 01.05.2020 21:27, schrieb Pablo Neira Ayuso: > On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote: >> + if (dtype == expr->dtype) >> + return; // do not free dtype before incrementing refcnt again > > The problem you describe (use-after-free) happens in this case, right? The problem is more likely due to concat_expr_parse_udata not calling datatype_get, because otherwise datatype_get would be in the backtrace of ASAN. > > datatype_set(expr, expr->dtype); > > Or am I missing anything? But while debugging the above output, I added assert(dtype != expr->dtype) here and that crashed. So I'm sure something like this is happening. And the whole thing was nasty to debug, so I added this one just be sure it does not happen again. As ASAN should hit on datatype_get incrementing refcnt if datatype_free had actually freed it, assert was probaby not seeing an DTYPE_F_ALLOC instance. But I dig not deeper here, as I felt this return is safe to add. > >> 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. This is what caused the ASAN output above to go away. Regards, M. Braun
On Fri, May 01, 2020 at 09:59:35PM +0200, michael-dev wrote: > Am 01.05.2020 21:27, schrieb Pablo Neira Ayuso: > > On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote: > > > + if (dtype == expr->dtype) > > > + return; // do not free dtype before incrementing refcnt again > > > > The problem you describe (use-after-free) happens in this case, right? > > The problem is more likely due to concat_expr_parse_udata not calling > datatype_get, > because otherwise datatype_get would be in the backtrace of ASAN. > > > > > datatype_set(expr, expr->dtype); > > > > Or am I missing anything? > > But while debugging the above output, I added assert(dtype != expr->dtype) > here > and that crashed. So I'm sure something like this is happening. Right. # nft add rule ip x y ct state new,established,related,untracked # nft list ruleset nft: datatype.c:1086: datatype_set: Assertion `expr->dtype != dtype' failed. Aborted > And the whole thing was nasty to debug, so I added this one just be sure it > does not happen again. > > As ASAN should hit on datatype_get incrementing refcnt if datatype_free had > actually freed it, > assert was probaby not seeing an DTYPE_F_ALLOC instance. > But I dig not deeper here, as I felt this return is safe to add. I'm going to apply this. I think it's safe to turn this into noop.
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 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); concat_expr->len = dtype->size; return concat_expr;
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(-)