ipt_DF: Fix for upstream replacement of skb_make_writable with skb_ensure_writable by lcrestez-dn · Pull Request #1 · drivenets/iptables_extensions

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


+#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


 	/* 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 <lcrestez@drivenets.com>

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 <lcrestez@drivenets.com>

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

Conversation

lcrestez-dn commented Sep 6, 2022

Uh oh!

lcrestez-dn commented Sep 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oshamash Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcrestez-dn commented Sep 7, 2022

Uh oh!

lschlesinger-dn commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oshamash commented Sep 7, 2022

Uh oh!

oshamash commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oshamash Sep 6, 2022 •

edited

Loading

lschlesinger-dn commented Sep 7, 2022 •

edited

Loading