[PATCH net] net: always initialize pagedlen

[PATCH net] net: always initialize pagedlen

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

In ip packet generation, pagedlen is initialized for each skb at the
start of the loop in __ip(6)_append_data, before label alloc_new_skb.

Depending on compiler options, code can be generated that jumps to
this label, triggering use of an an uninitialized variable.

In practice, at -O2, the generated code moves the initialization below
the label. But the code should not rely on that for correctness.

Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
Signed-off-by: Willem de Bruijn <willemb@google.com>

---

Noticed when rebasing udp zerocopy. I considered merging as part of
that patchset, but this seemed like it should go to net, even if it
does not trigger in practice.

Merged net with this patch onto net-next with udp zerocopy to verify
that there is no merge conflict.
---
 net/ipv4/ip_output.c  | 3 ++-
 net/ipv6/ip6_output.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index c09219e7f230..5dbec21856f4 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -939,7 +939,7 @@ static int __ip_append_data(struct sock *sk,
 			unsigned int fraglen;
 			unsigned int fraggap;
 			unsigned int alloclen;
-			unsigned int pagedlen = 0;
+			unsigned int pagedlen;
 			struct sk_buff *skb_prev;
 alloc_new_skb:
 			skb_prev = skb;
@@ -956,6 +956,7 @@ static int __ip_append_data(struct sock *sk,
 			if (datalen > mtu - fragheaderlen)
 				datalen = maxfraglen - fragheaderlen;
 			fraglen = datalen + fragheaderlen;
+			pagedlen = 0;
 
 			if ((flags & MSG_MORE) &&
 			    !(rt->dst.dev->features&NETIF_F_SG))
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 89e0d5118afe..827a3f5ff3bb 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1354,7 +1354,7 @@ static int __ip6_append_data(struct sock *sk,
 			unsigned int fraglen;
 			unsigned int fraggap;
 			unsigned int alloclen;
-			unsigned int pagedlen = 0;
+			unsigned int pagedlen;
 alloc_new_skb:
 			/* There's no room in the current skb */
 			if (skb)
@@ -1378,6 +1378,7 @@ static int __ip6_append_data(struct sock *sk,
 			if (datalen > (cork->length <= mtu && !(cork->flags & IPCORK_ALLFRAG) ? mtu : maxfraglen) - fragheaderlen)
 				datalen = maxfraglen - fragheaderlen - rt->dst.trailer_len;
 			fraglen = datalen + fragheaderlen;
+			pagedlen = 0;
 
 			if ((flags & MSG_MORE) &&
 			    !(rt->dst.dev->features&NETIF_F_SG))
-- 
2.20.0.rc0.387.gc7a69e6b6c-goog

Re: [PATCH net] net: always initialize pagedlen

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Sat, 24 Nov 2018 14:21:16 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> In ip packet generation, pagedlen is initialized for each skb at the
> start of the loop in __ip(6)_append_data, before label alloc_new_skb.
> 
> Depending on compiler options, code can be generated that jumps to
> this label, triggering use of an an uninitialized variable.
> 
> In practice, at -O2, the generated code moves the initialization below
> the label. But the code should not rely on that for correctness.
> 
> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> 
> ---
> 
> Noticed when rebasing udp zerocopy. I considered merging as part of
> that patchset, but this seemed like it should go to net, even if it
> does not trigger in practice.
> 
> Merged net with this patch onto net-next with udp zerocopy to verify
> that there is no merge conflict.

Ok, applied, but not queued for -stable.

Let me know if I should -stable this.

Re: [PATCH net] net: always initialize pagedlen

[Willem de Bruijn]

On Sat, Nov 24, 2018 at 8:43 PM David Miller <davem@davemloft.net> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Sat, 24 Nov 2018 14:21:16 -0500
>
> > From: Willem de Bruijn <willemb@google.com>
> >
> > In ip packet generation, pagedlen is initialized for each skb at the
> > start of the loop in __ip(6)_append_data, before label alloc_new_skb.
> >
> > Depending on compiler options, code can be generated that jumps to
> > this label, triggering use of an an uninitialized variable.
> >
> > In practice, at -O2, the generated code moves the initialization below
> > the label. But the code should not rely on that for correctness.
> >
> > Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
> > ---
> >
> > Noticed when rebasing udp zerocopy. I considered merging as part of
> > that patchset, but this seemed like it should go to net, even if it
> > does not trigger in practice.
> >
> > Merged net with this patch onto net-next with udp zerocopy to verify
> > that there is no merge conflict.
>
> Ok, applied, but not queued for -stable.
>
> Let me know if I should -stable this.

Thanks, David. No, that's not needed.