| Message ID | 20210902163205.17164-1-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux-next] ipv4: Fix NULL deference in fnhe_remove_oldest() | expand |
On Thu, Sep 2, 2021 at 9:32 AM Tim Gardner <tim.gardner@canonical.com> wrote: > > Coverity complains that linux-next commit 67d6d681e15b5 ("ipv4: make > exception cache less predictible") neglected to check for NULL before > dereferencing 'oldest'. It appears to be possible to fall through the for > loop without ever setting 'oldest'. Coverity is wrong. fnhe_remove_oldest() is only called when there are at least 6 items in the list. There is no way oldest could be NULL, or that oldest_p could contain garbage. > > Cc: Eric Dumazet <edumazet@google.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > Cc: David Ahern <dsahern@kernel.org> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > net/ipv4/route.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index d6899ab5fb39..e85026591a09 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -603,9 +603,11 @@ static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash) > oldest_p = fnhe_p; > } > } > - fnhe_flush_routes(oldest); > - *oldest_p = oldest->fnhe_next; > - kfree_rcu(oldest, rcu); > + if (oldest) { > + fnhe_flush_routes(oldest); > + *oldest_p = oldest->fnhe_next; > + kfree_rcu(oldest, rcu); > + } > } > > static u32 fnhe_hashfun(__be32 daddr) > -- > 2.33.0 >
On 9/2/21 9:38 AM, Eric Dumazet wrote: > On Thu, Sep 2, 2021 at 9:32 AM Tim Gardner <tim.gardner@canonical.com> wrote: >> >> Coverity complains that linux-next commit 67d6d681e15b5 ("ipv4: make >> exception cache less predictible") neglected to check for NULL before >> dereferencing 'oldest'. It appears to be possible to fall through the for >> loop without ever setting 'oldest'. > > Coverity is wrong. > > fnhe_remove_oldest() is only called when there are at least 6 items > in the list. > > There is no way oldest could be NULL, or that oldest_p could contain garbage. > +1
diff --git a/net/ipv4/route.c b/net/ipv4/route.c index d6899ab5fb39..e85026591a09 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -603,9 +603,11 @@ static void fnhe_remove_oldest(struct fnhe_hash_bucket *hash) oldest_p = fnhe_p; } } - fnhe_flush_routes(oldest); - *oldest_p = oldest->fnhe_next; - kfree_rcu(oldest, rcu); + if (oldest) { + fnhe_flush_routes(oldest); + *oldest_p = oldest->fnhe_next; + kfree_rcu(oldest, rcu); + } } static u32 fnhe_hashfun(__be32 daddr)
Coverity complains that linux-next commit 67d6d681e15b5 ("ipv4: make exception cache less predictible") neglected to check for NULL before dereferencing 'oldest'. It appears to be possible to fall through the for loop without ever setting 'oldest'. Cc: Eric Dumazet <edumazet@google.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> Cc: David Ahern <dsahern@kernel.org> Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- net/ipv4/route.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)