Re: [PATCH net 1/4] netfilter: xt_recent: Fix attempt to update deleted entry

[Jozsef Kadlecsik <kadlec@netfilter.org>]

Hi Harald,

On Fri, 5 Feb 2021, Reindl Harald wrote:

> "Reap only entries which won't be updated" sounds for me like the could 
> be some optimization: i mean when you first update and then check what 
> can be reaped the recently updated entry would not match to begin with

When the entry is new and the given recent table is full we cannot update 
(add) it, unless old entries are deleted (reaped) first. So it'd require 
more additional checkings to be introduced to reverse the order of the two 
operations.

Best regards,
Jozsef
 
> Am 05.02.21 um 01:17 schrieb Pablo Neira Ayuso:
> > From: Jozsef Kadlecsik <kadlec@mail.kfki.hu>
> > 
> > When both --reap and --update flag are specified, there's a code
> > path at which the entry to be updated is reaped beforehand,
> > which then leads to kernel crash. Reap only entries which won't be
> > updated.
> > 
> > Fixes kernel bugzilla #207773.
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=207773
> > Reported-by: Reindl Harald <h.reindl@thelounge.net>
> > Fixes: 0079c5aee348 ("netfilter: xt_recent: add an entry reaper")
> > Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >   net/netfilter/xt_recent.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
> > index 606411869698..0446307516cd 100644
> > --- a/net/netfilter/xt_recent.c
> > +++ b/net/netfilter/xt_recent.c
> > @@ -152,7 +152,8 @@ static void recent_entry_remove(struct recent_table *t,
> > struct recent_entry *e)
> >   /*
> >    * Drop entries with timestamps older then 'time'.
> >    */
> > -static void recent_entry_reap(struct recent_table *t, unsigned long time)
> > +static void recent_entry_reap(struct recent_table *t, unsigned long time,
> > +			      struct recent_entry *working, bool update)
> >   {
> >   	struct recent_entry *e;
> >   @@ -161,6 +162,12 @@ static void recent_entry_reap(struct recent_table *t,
> > unsigned long time)
> >   	 */
> >   	e = list_entry(t->lru_list.next, struct recent_entry, lru_list);
> >   +	/*
> > +	 * Do not reap the entry which are going to be updated.
> > +	 */
> > +	if (e == working && update)
> > +		return;
> > +
> >   	/*
> >   	 * The last time stamp is the most recent.
> >   	 */
> > @@ -303,7 +310,8 @@ recent_mt(const struct sk_buff *skb, struct
> > xt_action_param *par)
> >     		/* info->seconds must be non-zero */
> >   		if (info->check_set & XT_RECENT_REAP)
> > -			recent_entry_reap(t, time);
> > +			recent_entry_reap(t, time, e,
> > +				info->check_set & XT_RECENT_UPDATE && ret);
> >   	}
> >     	if (info->check_set & XT_RECENT_SET ||
> 

-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary