Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion extensions/dontfragment/ipt_DF.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#ifndef LINUX_VERSION_CODE
#include <linux/version.h>
#endif
#include <linux/module.h>
#include <linux/kernel.h>
#include <net/ip.h>
Expand All @@ -10,6 +13,14 @@ MODULE_AUTHOR("Semyon Verchenko");
MODULE_DESCRIPTION("Netfilter module to set/reset DF flag");
MODULE_LICENSE("Dual BSD/GPL");

/*
* skb_make_writable was removed in this commit:
* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=2cf6bffc49dae26edd12af6b57c8c780590380bf
*/
#if (LINUX_VERSION_CODE < KERNEL_VERSION(5,3,0))
#define skb_ensure_writable(skb, len) (!skb_make_writable((skb), (len)))
#endif

static int df_tg_check(const struct xt_tgchk_param *param)
{
return 0;
Expand All @@ -23,7 +34,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))){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))) {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link

@oshamash oshamash Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lschlesinger-dn @fpopescu-dn WDYT ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😛

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

printk(KERN_ERR "DF: Error making skb writable\n");
return NF_DROP;
}
Expand Down