| Message ID | 20191206033902.19638-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] gre: refetch erspan header from skb->data after pskb_may_pull() | expand |
Hi Cong, On Thu, Dec 05, 2019 at 07:39:02PM -0800, Cong Wang wrote: > After pskb_may_pull() we should always refetch the header > pointers from the skb->data in case it got reallocated. > > In gre_parse_header(), the erspan header is still fetched > from the 'options' pointer which is fetched before > pskb_may_pull(). > > Found this during code review of a KMSAN bug report. > > Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") > Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/ipv4/gre_demux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c > index 44bfeecac33e..5fd6e8ed02b5 100644 > --- a/net/ipv4/gre_demux.c > +++ b/net/ipv4/gre_demux.c > @@ -127,7 +127,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr))) > return -EINVAL; > > - ershdr = (struct erspan_base_hdr *)options; > + ershdr = (struct erspan_base_hdr *)(skb->data + nhs + hdr_len); It seems to me that in the case of WCCPv2 hdr_len will be 4 bytes longer than where options would be advanced to. Is that a problem here? > tpi->key = cpu_to_be32(get_session_id(ershdr)); > } > > -- > 2.21.0 >
> > After pskb_may_pull() we should always refetch the header > pointers from the skb->data in case it got reallocated. > > In gre_parse_header(), the erspan header is still fetched > from the 'options' pointer which is fetched before > pskb_may_pull(). > > Found this during code review of a KMSAN bug report. > > Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") > Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > net/ipv4/gre_demux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c > index 44bfeecac33e..5fd6e8ed02b5 100644 > --- a/net/ipv4/gre_demux.c > +++ b/net/ipv4/gre_demux.c > @@ -127,7 +127,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr))) > return -EINVAL; > > - ershdr = (struct erspan_base_hdr *)options; > + ershdr = (struct erspan_base_hdr *)(skb->data + nhs + hdr_len); > tpi->key = cpu_to_be32(get_session_id(ershdr)); > } > > -- > 2.21.0 >
> > Hi Cong, > > On Thu, Dec 05, 2019 at 07:39:02PM -0800, Cong Wang wrote: > > After pskb_may_pull() we should always refetch the header > > pointers from the skb->data in case it got reallocated. > > > > In gre_parse_header(), the erspan header is still fetched > > from the 'options' pointer which is fetched before > > pskb_may_pull(). > > > > Found this during code review of a KMSAN bug report. > > > > Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") > > Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > --- > > net/ipv4/gre_demux.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c > > index 44bfeecac33e..5fd6e8ed02b5 100644 > > --- a/net/ipv4/gre_demux.c > > +++ b/net/ipv4/gre_demux.c > > @@ -127,7 +127,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > > if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr))) > > return -EINVAL; > > > > - ershdr = (struct erspan_base_hdr *)options; > > + ershdr = (struct erspan_base_hdr *)(skb->data + nhs + hdr_len); > > It seems to me that in the case of WCCPv2 hdr_len will be 4 bytes longer > than where options would be advanced to. Is that a problem here? > Hi Simon, I guess the two conditions are mutually exclusive since tpi->proto is initialized with greh->protocol. Am I missing something? Regards, Lorenzo > > tpi->key = cpu_to_be32(get_session_id(ershdr)); > > } > > > > -- > > 2.21.0 > > >
On Fri, Dec 6, 2019 at 3:50 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > > > > After pskb_may_pull() we should always refetch the header > > pointers from the skb->data in case it got reallocated. > > > > In gre_parse_header(), the erspan header is still fetched > > from the 'options' pointer which is fetched before > > pskb_may_pull(). > > > > Found this during code review of a KMSAN bug report. > > > > Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") > > Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > LGTM. Acked-by: William Tu <u9012063@gmail.com> From the spec, ERSPAN has fixed size GRE header, so I think WCCPv2 should not exist in ERSPAN.
On Fri, Dec 06, 2019 at 01:55:25PM +0200, Lorenzo Bianconi wrote: > > > > Hi Cong, > > > > On Thu, Dec 05, 2019 at 07:39:02PM -0800, Cong Wang wrote: > > > After pskb_may_pull() we should always refetch the header > > > pointers from the skb->data in case it got reallocated. > > > > > > In gre_parse_header(), the erspan header is still fetched > > > from the 'options' pointer which is fetched before > > > pskb_may_pull(). > > > > > > Found this during code review of a KMSAN bug report. > > > > > > Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") > > > Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > > > --- > > > net/ipv4/gre_demux.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c > > > index 44bfeecac33e..5fd6e8ed02b5 100644 > > > --- a/net/ipv4/gre_demux.c > > > +++ b/net/ipv4/gre_demux.c > > > @@ -127,7 +127,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, > > > if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr))) > > > return -EINVAL; > > > > > > - ershdr = (struct erspan_base_hdr *)options; > > > + ershdr = (struct erspan_base_hdr *)(skb->data + nhs + hdr_len); > > > > It seems to me that in the case of WCCPv2 hdr_len will be 4 bytes longer > > than where options would be advanced to. Is that a problem here? > > > > Hi Simon, > > I guess the two conditions are mutually exclusive since tpi->proto is > initialized with greh->protocol. Am I missing something? Thanks Lorenzo, I see that now and agree that this patch is correct. Reviewed-by: Simon Horman <simon.horman@netronome.com> > > > tpi->key = cpu_to_be32(get_session_id(ershdr)); > > > } > > > > > > -- > > > 2.21.0 > > > > > >
From: Cong Wang <xiyou.wangcong@gmail.com> Date: Thu, 5 Dec 2019 19:39:02 -0800 > After pskb_may_pull() we should always refetch the header > pointers from the skb->data in case it got reallocated. > > In gre_parse_header(), the erspan header is still fetched > from the 'options' pointer which is fetched before > pskb_may_pull(). > > Found this during code review of a KMSAN bug report. > > Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") > Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Applied and queued up for -stable, thanks.
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c index 44bfeecac33e..5fd6e8ed02b5 100644 --- a/net/ipv4/gre_demux.c +++ b/net/ipv4/gre_demux.c @@ -127,7 +127,7 @@ int gre_parse_header(struct sk_buff *skb, struct tnl_ptk_info *tpi, if (!pskb_may_pull(skb, nhs + hdr_len + sizeof(*ershdr))) return -EINVAL; - ershdr = (struct erspan_base_hdr *)options; + ershdr = (struct erspan_base_hdr *)(skb->data + nhs + hdr_len); tpi->key = cpu_to_be32(get_session_id(ershdr)); }
After pskb_may_pull() we should always refetch the header pointers from the skb->data in case it got reallocated. In gre_parse_header(), the erspan header is still fetched from the 'options' pointer which is fetched before pskb_may_pull(). Found this during code review of a KMSAN bug report. Fixes: cb73ee40b1b3 ("net: ip_gre: use erspan key field for tunnel lookup") Cc: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/ipv4/gre_demux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)