| Message ID | 346934f2ad88d64589fa9a942aed844443cf7110.1634028240.git.lucien.xin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6 | expand |
Xin Long <lucien.xin@gmail.com> wrote: > In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer() > only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to. > The access by ((const struct rt0_hdr *)rh)->reserved will overflow > the buffer. So this access should be moved below the 2nd call to > skb_header_pointer(). > > Besides, after the 2nd skb_header_pointer(), its return value should > also be checked, othersize, *rp may cause null-pointer-ref. Patch looks good but I think you can just axe these pr_debug statements instead of moving them. Before pr_debug conversion these statments were #if-0 out, I don't think they'll be missed if they are removed.
On Tue, Oct 12, 2021 at 12:02:04PM +0200, Florian Westphal wrote: > Xin Long <lucien.xin@gmail.com> wrote: > > In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer() > > only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to. > > The access by ((const struct rt0_hdr *)rh)->reserved will overflow > > the buffer. So this access should be moved below the 2nd call to > > skb_header_pointer(). > > > > Besides, after the 2nd skb_header_pointer(), its return value should > > also be checked, othersize, *rp may cause null-pointer-ref. > > Patch looks good but I think you can just axe these pr_debug statements > instead of moving them. > > Before pr_debug conversion these statments were #if-0 out, I don't think > they'll be missed if they are removed. Agreed.
On Tue, Oct 12, 2021 at 6:30 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Tue, Oct 12, 2021 at 12:02:04PM +0200, Florian Westphal wrote: > > Xin Long <lucien.xin@gmail.com> wrote: > > > In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer() > > > only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to. > > > The access by ((const struct rt0_hdr *)rh)->reserved will overflow > > > the buffer. So this access should be moved below the 2nd call to > > > skb_header_pointer(). > > > > > > Besides, after the 2nd skb_header_pointer(), its return value should > > > also be checked, othersize, *rp may cause null-pointer-ref. > > > > Patch looks good but I think you can just axe these pr_debug statements > > instead of moving them. > > > > Before pr_debug conversion these statments were #if-0 out, I don't think > > they'll be missed if they are removed. > > Agreed. Posted v2. Thanks.
diff --git a/net/ipv6/netfilter/ip6t_rt.c b/net/ipv6/netfilter/ip6t_rt.c index 733c83d38b30..d25192949217 100644 --- a/net/ipv6/netfilter/ip6t_rt.c +++ b/net/ipv6/netfilter/ip6t_rt.c @@ -83,11 +83,7 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par) !(rtinfo->flags & IP6T_RT_LEN) || ((rtinfo->hdrlen == hdrlen) ^ !!(rtinfo->invflags & IP6T_RT_INV_LEN))); - pr_debug("res %02X %02X %02X ", - rtinfo->flags & IP6T_RT_RES, - ((const struct rt0_hdr *)rh)->reserved, - !((rtinfo->flags & IP6T_RT_RES) && - (((const struct rt0_hdr *)rh)->reserved))); + pr_debug("res flag %02X ", rtinfo->flags & IP6T_RT_RES); ret = (segsleft_match(rtinfo->segsleft[0], rtinfo->segsleft[1], rh->segments_left, @@ -107,7 +103,12 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par) reserved), sizeof(_reserved), &_reserved); + if (!rp) { + par->hotdrop = true; + return false; + } + pr_debug("res value %02X ", *rp); ret = (*rp == 0); }
In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer() only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to. The access by ((const struct rt0_hdr *)rh)->reserved will overflow the buffer. So this access should be moved below the 2nd call to skb_header_pointer(). Besides, after the 2nd skb_header_pointer(), its return value should also be checked, othersize, *rp may cause null-pointer-ref. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/ipv6/netfilter/ip6t_rt.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)