Patchwork bridge netfilter output bug on 2.6.39

login
register
mail settings
Submitter stephen hemminger
Date May 24, 2011, 5:40 p.m.
Message ID <20110524104022.212b0b71@nehalam>
Download mbox | patch
Permalink /patch/97188/
State Accepted
Delegated to: David Miller
Headers show

Comments

stephen hemminger - May 24, 2011, 5:40 p.m.
On Tue, 24 May 2011 18:46:27 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le mardi 24 mai 2011 à 18:27 +0200, Eric Dumazet a écrit :
> > Le mardi 24 mai 2011 à 17:39 +0200, Eric Dumazet a écrit :
> > 
> > > I would say its more likely a problem with dst metrics changes
> > > 
> > > In this crash, we dereference a NULL dst->_metrics 'pointer' in
> > > dst_metric_raw(dst, RTAX_MTU);
> > 
> > It seems bridge code uses one fake_rtable
> > 
> > You probably want to properly init its _metric field.
> > 
> > I can do the patch in one hour eventually, if nobody beats me.
> > 
> > 
> 
> Here is the patch :
> 
> [PATCH] bridge: initialize fake_rtable metrics
> 
> bridge netfilter code uses a fake_rtable, and we must init its _metric
> field or risk NULL dereference later.
> 
> Ref: https://bugzilla.kernel.org/show_bug.cgi?id=35672
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Stephen Hemminger <shemminger@vyatta.com>
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  net/bridge/br_netfilter.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index e1f5ec7..3fa1231 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -117,6 +117,10 @@ static struct dst_ops fake_dst_ops = {
>   * ipt_REJECT needs it.  Future netfilter modules might
>   * require us to fill additional fields.
>   */
> +static const u32 br_dst_default_metrics[RTAX_MAX] = {
> +	[RTAX_MTU - 1] = 1500,
> +};
> +
>  void br_netfilter_rtable_init(struct net_bridge *br)
>  {
>  	struct rtable *rt = &br->fake_rtable;
> @@ -124,7 +128,7 @@ void br_netfilter_rtable_init(struct net_bridge *br)
>  	atomic_set(&rt->dst.__refcnt, 1);
>  	rt->dst.dev = br->dev;
>  	rt->dst.path = &rt->dst;
> -	dst_metric_set(&rt->dst, RTAX_MTU, 1500);
> +	dst_init_metrics(&rt->dst, br_dst_default_metrics, true);
>  	rt->dst.flags	= DST_NOXFRM;
>  	rt->dst.ops = &fake_dst_ops;
>  }

This part is fine.

Acked-by: Stephen Hemminger <shemminger@vyatta.com>

I think there should be BUG_ON any calls to dst_metric_set where
dst has no metrics available.

[PATCH] dst: catch uninitialized metrics

Catch cases where dst_metric_set() and other functions are called
but _metrics is NULL.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 24, 2011, 5:49 p.m.
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 24 May 2011 10:40:22 -0700

> I think there should be BUG_ON any calls to dst_metric_set where
> dst has no metrics available.

Yeah I'll add this too, we can kill it later after we're sure that
we've snuffed out all of these kinds of bugs.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/include/net/dst.h	2011-05-24 10:36:07.597962703 -0700
+++ b/include/net/dst.h	2011-05-24 10:36:54.382509111 -0700
@@ -111,6 +111,8 @@  static inline u32 *dst_metrics_write_ptr
 {
 	unsigned long p = dst->_metrics;
 
+	BUG_ON(!p);
+
 	if (p & DST_METRICS_READ_ONLY)
 		return dst->ops->cow_metrics(dst, p);
 	return __DST_METRICS_PTR(p);