2022-12-02 09:30:41

by Li Qiong

Subject: [PATCH] netfilter: nfnetlink: check 'skb->dev' pointer in nfulnl_log_packet()

The 'skb->dev' may be NULL, it should be better to check it.

Signed-off-by: Li Qiong <[email protected]>
---
net/netfilter/nfnetlink_log.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index d97eb280cb2e..74ac9fa40137 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -572,7 +572,7 @@ __build_packet_message(struct nfnl_log_net *log,
}
}

- if (indev && skb_mac_header_was_set(skb)) {
+ if (indev && skb->dev && skb_mac_header_was_set(skb)) {
if (nla_put_be16(inst->skb, NFULA_HWTYPE, htons(skb->dev->type)) ||
nla_put_be16(inst->skb, NFULA_HWLEN,
htons(skb->dev->hard_header_len)))
@@ -724,7 +724,7 @@ nfulnl_log_packet(struct net *net,
+ nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp))
+ nla_total_size(sizeof(struct nfgenmsg)); /* NLMSG_DONE */

- if (in && skb_mac_header_was_set(skb)) {
+ if (in && skb->dev && skb_mac_header_was_set(skb)) {
size += nla_total_size(skb->dev->hard_header_len)
+ nla_total_size(sizeof(u_int16_t)) /* hwtype */
+ nla_total_size(sizeof(u_int16_t)); /* hwlen */
--
2.11.0

2022-12-02 11:12:21

by Dan Carpenter

Subject: Re: [PATCH] netfilter: nfnetlink: check 'skb->dev' pointer in nfulnl_log_packet()

On Fri, Dec 02, 2022 at 04:33:04PM +0800, Li Qiong wrote:
> The 'skb->dev' may be NULL, it should be better to check it.
>
> Signed-off-by: Li Qiong <[email protected]>
> ---

This needs a Fixes tag as well.

Here my records show that I looked at this in 2018 but I probably
ignored it based on that the code was introduced in 2012. When warnings
are really ancient sometimes I just assume they must be false positives
or someone would have hit it in testing.

Also for really ancient bugs, it's hard to contact the original author
or they have forgotten the details of the code.

regards,
dan carpenter

2022-12-05 14:53:49

by Pablo Neira Ayuso

Subject: Re: [PATCH] netfilter: nfnetlink: check 'skb->dev' pointer in nfulnl_log_packet()

On Fri, Dec 02, 2022 at 04:33:04PM +0800, Li Qiong wrote:
> The 'skb->dev' may be NULL, it should be better to check it.
>
> Signed-off-by: Li Qiong <[email protected]>
> ---
> net/netfilter/nfnetlink_log.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index d97eb280cb2e..74ac9fa40137 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -572,7 +572,7 @@ __build_packet_message(struct nfnl_log_net *log,
> }
> }
>
> - if (indev && skb_mac_header_was_set(skb)) {
> + if (indev && skb->dev && skb_mac_header_was_set(skb)) {
> if (nla_put_be16(inst->skb, NFULA_HWTYPE, htons(skb->dev->type)) ||
> nla_put_be16(inst->skb, NFULA_HWLEN,
> htons(skb->dev->hard_header_len)))
> @@ -724,7 +724,7 @@ nfulnl_log_packet(struct net *net,
> + nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp))
> + nla_total_size(sizeof(struct nfgenmsg)); /* NLMSG_DONE */
>
> - if (in && skb_mac_header_was_set(skb)) {
> + if (in && skb->dev && skb_mac_header_was_set(skb)) {
> size += nla_total_size(skb->dev->hard_header_len)
> + nla_total_size(sizeof(u_int16_t)) /* hwtype */
> + nla_total_size(sizeof(u_int16_t)); /* hwlen */

skb->dev is always guaranteed to be set in this path.