ipt_DF: Fix for upstream replacement of skb_make_writable with skb_ensure_writable #1

lcrestez-dn · 2022-09-06T14:25:54Z

No description provided.

lcrestez-dn · 2022-09-06T14:27:36Z

lschlesinger-dn · 2022-09-06T14:44:36Z

extensions/dontfragment/ipt_DF.c

@@ -10,6 +13,12 @@ MODULE_AUTHOR("Semyon Verchenko");
 MODULE_DESCRIPTION("Netfilter module to set/reset DF flag");
 MODULE_LICENSE("Dual BSD/GPL");

+#if (LINUX_VERSION_CODE < KERNEL_VERSION(5,2,0))
+# skb_make_writable was removed in this commit:
+# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=2cf6bffc49dae26edd12af6b57c8c780590380bf


Comments should begin with /* :-)

Also... i see skb_ensure_writeable is present in 4.15. Is this needed ?

@fpopescu-dn Using skb_ensure_writeable() was actually a bug, and so it appears we need to use skb_make_writable() drivenets/dontfragment@38e1b4d
+ @oshamash might have more info

I remember now @oshamash pinged me about this some time ago (spending days to find this). So why it would work now in the 5.2+

Sorry, it was this drivenets/dontfragment@94d2273

Comments should begin with /* :-)

Fixed

I made a dumb compat fix assuming code is already correct. Seeing how it doesn't even compile on 5.4 it might be unused.

lschlesinger-dn · 2022-09-06T14:48:12Z

extensions/dontfragment/ipt_DF.c

@@ -23,7 +32,7 @@ static unsigned int df_tg(struct sk_buff *skb, const struct xt_action_param *par
 	__u16 old_frag_off, new_frag_off;

 	/* make_writable might invoke copy-on-write, so fetch iph afterwards */
-	if (!skb_make_writable(skb, sizeof(struct iphdr))){
+	if (skb_ensure_writable(skb, sizeof(struct iphdr))){


IMO it will be more readable to have the #if (LINUX_VERSION_CODE < KERNEL_VERSION(5,2,0)) here, instead of making skb_ensure_writable a macro (if someone casually goes over the code he will be surprised that skb_ensure_writable() is a macro).

#if (LINUX_VERSION_CODE < KERNEL_VERSION(5,2,0)) if (!skb_make_writable(skb, sizeof(struct iphdr))){ #else if (skb_ensure_writable(skb, sizeof(struct iphdr))){ #endif

Using only the new version in code means that there is a single place where you can drop support for old versions.

I agree w/ @lschlesinger-dn here - it's mainly about readability,
if you assume macro == function in some versions, it's malpractice

instead you can go with

#if (LINUX_VERSION_CODE < KERNEL_VERSION(5,2,0)) #define DN_ENSURE_WRITABLE(skb, len) (!skb_make_writable((skb), (len))) #else #define DN_ENSURE_WRITABLE(skb, len) (skb_ensure_writable((skb), (len)) #endif .... if (DN_ENSURE_WRITABLE(skb, sizeof(struct iphdr))) {

as for why - the issue is that sometimes skb needs to be reallocated to allow writing changes, this makes the previously fetched header-pointer a UAF bug ("use-after-free")

another note per what @fpopescu-dn mentioned, I do remember trying to initially use ensure_writable and it failed on older kernel - it would just not fix the IP packet and would avoid the patch altogether as it was required to forcefully make the SKB writable

So honestly not sure if we can really use this in new kernel - QA don't even have a proper scenario to test patch

EDIT: thinking back again, I think initially I changed to https://www.kernel.org/doc/htmldocs/networking/API-skb-clone-writable.html then when it kept failing - I kept it as make_writable

now going over kernel code - there is no reason to distinguish the KERNEL version, as the skb_ensure_writable is coming from SKB_CORE code, while skb_make_writable comes from NETFILTER_CORE, it was a development hindsight that they missed this duplication, while they do operate a little differently, they both guarantee same thing

@lschlesinger-dn @fpopescu-dn WDYT ?

I also think it's probably safe to just stick with skb_ensure_writable, but given that QA indeed can't test it then my paranoid side says we shouldn't touch the old code which we know works 😛

i vote to keep the old version skb_make_writable but also for kernel 5.4 which we already use and we know that it works. For versions > 5.4 we can go with the skb_ensure_writable and pray it does the same thing (the code is a little different, but in the end the result should be the same).

…sure_writable Signed-off-by: Leonard Crestez <[email protected]>

lcrestez-dn · 2022-09-07T07:12:36Z

The skb_make_writable function doesn't even exist in linux 5.4, this means that this module is only used for linux 4.15?!?!

I will just skip it for building because it seems to me to be unused.

lschlesinger-dn · 2022-09-07T07:29:47Z

The skb_make_writable function doesn't even exist in linux 5.4, this means that this module is only used for linux 4.15?!?!

I will just skip it for building because it seems to me to be unused.

$ nm image/iptables_extensions/5.4.0-73-generic/ipt_DF.ko | grep skb_
                 U skb_ensure_writable

nice
@lcrestez-dn @oshamash @fpopescu-dn

oshamash · 2022-09-07T07:51:57Z

The skb_make_writable function doesn't even exist in linux 5.4, this means that this module is only used for linux 4.15?!?!

I will just skip it for building because it seems to me to be unused.

this module is needed for iptables to allow -j DF target, see containers/golden_image/scripts/replace_vxlan.sh

# Note: host_ns exist here due to being created prior in entrypoint
if ! ${NETNS_VXLAN_EXEC} /sbin/ip link show dev ${VXLAN_DEV} 2>/dev/null; then
  wait_for_ctrl_bond
  ...
  log_debug "Adding rule for iptables-df"
  ${NETNS_VXLAN_EXEC} /sbin/iptables -t mangle -A FORWARD -j DF --reset
  ${NETNS_VXLAN_EXEC} /sbin/iptables -t mangle -A POSTROUTING -j DF --reset

oshamash · 2022-09-07T07:53:46Z

@lcrestez-dn also, the skb_ensure_writable is available both on 4.15 and 5.4,
so the entire MACRO thing is not good if we want to make older version !

skb_make_writable was removed in newer kernels since it was a duplicate logic of skb_ensure_writable

lschlesinger-dn reviewed Sep 6, 2022

View reviewed changes

ipt_DF: Fix for upstream replacement of skb_make_writable with skb_en…

364bcd5

…sure_writable Signed-off-by: Leonard Crestez <[email protected]>

lcrestez-dn force-pushed the lcrestez/compat_fix branch from 1861d0a to 364bcd5 Compare September 6, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipt_DF: Fix for upstream replacement of skb_make_writable with skb_ensure_writable #1

ipt_DF: Fix for upstream replacement of skb_make_writable with skb_ensure_writable #1

lcrestez-dn commented Sep 6, 2022

lcrestez-dn commented Sep 6, 2022

lschlesinger-dn Sep 6, 2022

fpopescu-dn Sep 6, 2022

lschlesinger-dn Sep 6, 2022

fpopescu-dn Sep 6, 2022

fpopescu-dn Sep 6, 2022

lcrestez-dn Sep 6, 2022

lcrestez-dn Sep 6, 2022

lschlesinger-dn Sep 6, 2022

lcrestez-dn Sep 6, 2022

oshamash Sep 6, 2022

oshamash Sep 6, 2022

oshamash Sep 6, 2022 •

edited

Loading

oshamash Sep 6, 2022

lschlesinger-dn Sep 6, 2022

fpopescu-dn Sep 6, 2022

lcrestez-dn commented Sep 7, 2022

lschlesinger-dn commented Sep 7, 2022 •

edited

Loading

oshamash commented Sep 7, 2022

oshamash commented Sep 7, 2022

ipt_DF: Fix for upstream replacement of skb_make_writable with skb_ensure_writable #1

Are you sure you want to change the base?

ipt_DF: Fix for upstream replacement of skb_make_writable with skb_ensure_writable #1

Conversation

lcrestez-dn commented Sep 6, 2022

lcrestez-dn commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oshamash Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcrestez-dn commented Sep 7, 2022

lschlesinger-dn commented Sep 7, 2022 • edited Loading

oshamash commented Sep 7, 2022

oshamash commented Sep 7, 2022

oshamash Sep 6, 2022 •

edited

Loading

lschlesinger-dn commented Sep 7, 2022 •

edited

Loading