| Message ID | 5ba67c28-1056-e24d-cad3-4b7aaac01111@virtuozzo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [NET] vrf: fix null pointer dereference in vrf_finish_output() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/header_inline | success | Link |
On Fri, 6 Aug 2021 15:53:00 +0300 Vasily Averin wrote: > After 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output") > skb->dev is accessed after skb free. > Let's replace skb->dev by dev = skb_dst(skb)->dev: > vrf_finish_output() is only called from vrf_output(), > it set skb->dev to skb_dst(skb)->dev and calls POSTROUTING netfilter > hooks, where output device should not be changed. > > Fixes: 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output") > Reported-by: Julian Wiedmann <jwi@linux.ibm.com> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> Thanks for following up! I decided to pick a similar patch from Dan Carpenter [1] because the chunk quoted below is not really necessary. [1] https://lore.kernel.org/kernel-janitors/20210806150435.GB15586@kili/ > @@ -883,7 +883,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s > } > > rcu_read_unlock_bh(); > - vrf_tx_error(skb->dev, skb); > + vrf_tx_error(dev, skb); > return -EINVAL; > } >
On 8/7/21 1:42 AM, Jakub Kicinski wrote: > On Fri, 6 Aug 2021 15:53:00 +0300 Vasily Averin wrote: >> After 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output") >> skb->dev is accessed after skb free. >> Let's replace skb->dev by dev = skb_dst(skb)->dev: >> vrf_finish_output() is only called from vrf_output(), >> it set skb->dev to skb_dst(skb)->dev and calls POSTROUTING netfilter >> hooks, where output device should not be changed. >> >> Fixes: 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output") >> Reported-by: Julian Wiedmann <jwi@linux.ibm.com> >> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > > Thanks for following up! I decided to pick a similar patch from Dan > Carpenter [1] because the chunk quoted below is not really necessary. I still think that my patch version is preferable. It's better to use vrf_tx_error(dev, skb) because: a) both rollbacks can use the same net device b) probably using 'dev' allows to avoid an extra pointer dereference. Originally, i.e. before fixed patch 14ee70ca89e6, rollback after failed header expand called the save vrf_tx_error() call. This function does 2 things: - increments stats.tx_errors on specified network device - frees provided skb. Commit 14ee70ca89e6 replaced skb_realloc_headroom() by skb_expand_head() that frees skb inside, So vrf_tx_error() call on rollback was replaced with direct increment of stats.tx_errors. We cannot use now original skb->dev so our fixup patches replaces it with dev variable already used in this function. Though, if we should use the same net device in both rollbacks. It's illogical for me to change one place and do not change another one. If we follow to your decision -- it isn't a problem. skb->dev and skb should be identical. Though 'skb->dev' does an extra dereference, while dev was used in function and probably was saved to register. Thank you, Vasily Averin > [1] https://lore.kernel.org/kernel-janitors/20210806150435.GB15586@kili/ > >> @@ -883,7 +883,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s >> } >> >> rcu_read_unlock_bh(); >> - vrf_tx_error(skb->dev, skb); >> + vrf_tx_error(dev, skb); >> return -EINVAL; >> } >> >
diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c index 726adf0..168d4ef 100644 --- a/drivers/net/vrf.c +++ b/drivers/net/vrf.c @@ -864,7 +864,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { skb = skb_expand_head(skb, hh_len); if (!skb) { - skb->dev->stats.tx_errors++; + dev->stats.tx_errors++; return -ENOMEM; } } @@ -883,7 +883,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s } rcu_read_unlock_bh(); - vrf_tx_error(skb->dev, skb); + vrf_tx_error(dev, skb); return -EINVAL; }
After 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output") skb->dev is accessed after skb free. Let's replace skb->dev by dev = skb_dst(skb)->dev: vrf_finish_output() is only called from vrf_output(), it set skb->dev to skb_dst(skb)->dev and calls POSTROUTING netfilter hooks, where output device should not be changed. Fixes: 14ee70ca89e6 ("vrf: use skb_expand_head in vrf_finish_output") Reported-by: Julian Wiedmann <jwi@linux.ibm.com> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> --- drivers/net/vrf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)