| Message ID | c49c716a-e070-4ad5-9a90-896537bcd1b5@web.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: nf_tables: Add a null pointer check in two functions | expand |
Hi Markus, On Tue, Jan 23, 2024 at 02:45:12PM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Tue, 23 Jan 2024 14:28:31 +0100 > > The result from a call of the function “kasprintf” was passed to > a subsequent function call without checking for a null pointer before > (according to a memory allocation failure). > This issue was detected by using the Coccinelle software. This is correct and I'm fine with the patch if it avoids ringing alarm bells somewhere, yet it doesn't fix an actual issue here since the allocated buffer is merely passed to vsnprintf() which detects and sanitizes %s args being NULL. Cheers, Phil
>> The result from a call of the function “kasprintf” was passed to >> a subsequent function call without checking for a null pointer before >> (according to a memory allocation failure). >> This issue was detected by using the Coccinelle software. > > This is correct and I'm fine with the patch if it avoids ringing alarm > bells somewhere, yet it doesn't fix an actual issue here since the > allocated buffer is merely passed to vsnprintf() which detects and > sanitizes %s args being NULL. Should null pointer tolerance be better indicated for such use cases by any additional means? Regards, Markus
On Tue, Jan 23, 2024 at 02:45:12PM +0100, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Tue, 23 Jan 2024 14:28:31 +0100 > > The result from a call of the function “kasprintf” was passed to > a subsequent function call without checking for a null pointer before > (according to a memory allocation failure). > This issue was detected by using the Coccinelle software. > > Thus add a null pointer check and a jump target in affected functions. > > Fixes: 8877393029e76 ("netfilter: nf_tables: Open-code audit log call in nf_tables_getrule()") > Fixes: 0854db2aaef3f ("netfilter: nf_tables: use net_generic infra for transaction data") > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> For the record: Markus has posed patches of this nature many times over the past weeks and months. They are not fixing bugs - there is no user visible problem. And moreover they move the code away from the preferred style - avoiding unnecessary checks around code that can handle NULL values. Markus, please stop posting patches of this nature.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 8438a8922e4a..cb61c7a39a76 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3728,10 +3728,13 @@ static int nf_tables_getrule_reset(struct sk_buff *skb, nla_len(nla[NFTA_RULE_TABLE]), (char *)nla_data(nla[NFTA_RULE_TABLE]), nft_net->base_seq); + if (!buf) + goto exit; + audit_log_nfcfg(buf, info->nfmsg->nfgen_family, 1, AUDIT_NFT_OP_RULE_RESET, GFP_ATOMIC); kfree(buf); - +exit: return nfnetlink_unicast(skb2, net, portid); } @@ -7917,6 +7920,8 @@ static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info, nft_net = nft_pernet(net); buf = kasprintf(GFP_ATOMIC, "%s:%u", table->name, nft_net->base_seq); + if (!buf) + goto fill_obj_info; audit_log_nfcfg(buf, family, @@ -7925,7 +7930,7 @@ static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info, GFP_ATOMIC); kfree(buf); } - +fill_obj_info: err = nf_tables_fill_obj_info(skb2, net, NETLINK_CB(skb).portid, info->nlh->nlmsg_seq, NFT_MSG_NEWOBJ, 0, family, table, obj, reset);