| Message ID | 80dbe764b5ae660bba3cf6edcb045a74b0f85853.1618844973.git.dcaratti@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix stack OOB read while fragmenting IPv4 packets | expand |
On Tue, Apr 20, 2021 at 1:59 AM Davide Caratti <dcaratti@redhat.com> wrote: > > hello Cong, thanks for looking at this! > > On Mon, 2021-04-19 at 11:46 -0700, Cong Wang wrote: > > On Mon, Apr 19, 2021 at 8:24 AM Davide Caratti <dcaratti@redhat.com> wrote: > > > diff --git a/net/sched/sch_frag.c b/net/sched/sch_frag.c > > > index e1e77d3fb6c0..8c06381391d6 100644 > > > --- a/net/sched/sch_frag.c > > > +++ b/net/sched/sch_frag.c > > > @@ -90,16 +90,16 @@ static int sch_fragment(struct net *net, struct sk_buff *skb, > > > } > > > > > > if (skb_protocol(skb, true) == htons(ETH_P_IP)) { > > > - struct dst_entry sch_frag_dst; > > > + struct rtable sch_frag_rt = { 0 }; > > > > Is setting these fields 0 sufficient here? Because normally a struct table > > is initialized by rt_dst_alloc() which sets several of them to non-zero, > > notably, rt->rt_type and rt->rt_uncached. > > > > Similar for the IPv6 part, which is initialized by rt6_info_init(). > > for what we do now in openvswitch and sch_frag, that should be > sufficient: a similar thing is done by br_netfilter [1], apparently for > the same "refragmentation" purposes. On a fedora host (running 5.10, but > it shouldn't be much different than current Linux), I just dumped > 'fake_rtable' from a bridge device: Sounds fair. It looks like all of these cases merely use dst->dev->mtu, so it is overkill to pass a full dst just to satisfy some callees. Anyway, your patch should be okay as a fix for -net. Thanks.
On Mon, Apr 19, 2021 at 8:24 AM Davide Caratti <dcaratti@redhat.com> wrote: > > when 'act_mirred' tries to fragment IPv4 packets that had been previously > re-assembled using 'act_ct', splats like the following can be observed on > kernels built with KASAN: [...] > for IPv4 packets, sch_fragment() uses a temporary struct dst_entry. Then, > in the following call graph: > > ip_do_fragment() > ip_skb_dst_mtu() > ip_dst_mtu_maybe_forward() > ip_mtu_locked() > > the pointer to struct dst_entry is used as pointer to struct rtable: this > turns the access to struct members like rt_mtu_locked into an OOB read in > the stack. Fix this changing the temporary variable used for IPv4 packets > in sch_fragment(), similarly to what is done for IPv6 few lines below. > > Fixes: c129412f74e9 ("net/sched: sch_frag: add generic packet fragment support.") > Cc: <stable@vger.kernel.org> # 5.11 > Reported-by: Shuang Li <shuali@redhat.com> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> Acked-by: Cong Wang <cong.wang@bytedance.com> Thanks.
diff --git a/net/sched/sch_frag.c b/net/sched/sch_frag.c index e1e77d3fb6c0..8c06381391d6 100644 --- a/net/sched/sch_frag.c +++ b/net/sched/sch_frag.c @@ -90,16 +90,16 @@ static int sch_fragment(struct net *net, struct sk_buff *skb, } if (skb_protocol(skb, true) == htons(ETH_P_IP)) { - struct dst_entry sch_frag_dst; + struct rtable sch_frag_rt = { 0 }; unsigned long orig_dst; sch_frag_prepare_frag(skb, xmit); - dst_init(&sch_frag_dst, &sch_frag_dst_ops, NULL, 1, + dst_init(&sch_frag_rt.dst, &sch_frag_dst_ops, NULL, 1, DST_OBSOLETE_NONE, DST_NOCOUNT); - sch_frag_dst.dev = skb->dev; + sch_frag_rt.dst.dev = skb->dev; orig_dst = skb->_skb_refdst; - skb_dst_set_noref(skb, &sch_frag_dst); + skb_dst_set_noref(skb, &sch_frag_rt.dst); IPCB(skb)->frag_max_size = mru; ret = ip_do_fragment(net, skb->sk, skb, sch_frag_xmit);