[PATCH V3] net: rose: Prevent the use of freed digipeat

[PATCH V3] net: rose: Prevent the use of freed digipeat

[Lizhi Xu]

There is no synchronization between the two timers, rose_t0timer_expiry
and rose_timer_expiry.
rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
However, rose_t0timer_expiry() does initiate a restart request on the
neighbor.
When rose_t0timer_expiry() accesses the released neighbor member digipeat,
a UAF is triggered.

To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
stop restarting t0timer after putting the neighbor.

When putting the neighbor, set the neighbor to NULL. Setting neighbor to
NULL prevents rose_t0timer_expiry() from restarting t0timer.

syzbot reported a slab-use-after-free Read in ax25_find_cb.
BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
Call Trace:
 ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
 ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
 rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
 rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
 rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83

Freed by task 17183:
 kfree+0x2b8/0x6d0 mm/slub.c:6826
 rose_neigh_put include/net/rose.h:165 [inline]
 rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183

Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: Putting the neighbor stops t0timer from automatically starting
V2 -> V3: add rose_neigh_putex for set rose neigh to NULL

 include/net/rose.h   | 12 ++++++++++++
 net/rose/rose_link.c |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/include/net/rose.h b/include/net/rose.h
index 2b5491bbf39a..33de310ba778 100644
--- a/include/net/rose.h
+++ b/include/net/rose.h
@@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
 	}
 }
 
+static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
+{
+	struct rose_neigh *rose_neigh = *roseneigh;
+	if (refcount_dec_and_test(&rose_neigh->use)) {
+		if (rose_neigh->ax25)
+			ax25_cb_put(rose_neigh->ax25);
+		kfree(rose_neigh->digipeat);
+		kfree(rose_neigh);
+		*roseneigh = NULL;
+	}
+}
+
 /* af_rose.c */
 extern ax25_address rose_callsign;
 extern int  sysctl_rose_restart_request_timeout;
diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
index 7746229fdc8c..334c8cc0876d 100644
--- a/net/rose/rose_link.c
+++ b/net/rose/rose_link.c
@@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
 
 static void rose_start_t0timer(struct rose_neigh *neigh)
 {
+	if (!neigh)
+		return;
+
 	timer_delete(&neigh->t0timer);
 
 	neigh->t0timer.function = rose_t0timer_expiry;
@@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
 {
 	struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
 
+	rose_neigh_hold(neigh);
 	rose_transmit_restart_request(neigh);
 
 	neigh->dce_mode = 0;
 
+	rose_neigh_putex(&neigh);
 	rose_start_t0timer(neigh);
 }
 
-- 
2.43.0

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Eric Dumazet]

On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> There is no synchronization between the two timers, rose_t0timer_expiry
> and rose_timer_expiry.
> rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> However, rose_t0timer_expiry() does initiate a restart request on the
> neighbor.
> When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> a UAF is triggered.
>
> To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> stop restarting t0timer after putting the neighbor.
>
> When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> NULL prevents rose_t0timer_expiry() from restarting t0timer.
>
> syzbot reported a slab-use-after-free Read in ax25_find_cb.
> BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> Call Trace:
>  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
>  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
>  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
>  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
>  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
>
> Freed by task 17183:
>  kfree+0x2b8/0x6d0 mm/slub.c:6826
>  rose_neigh_put include/net/rose.h:165 [inline]
>  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
>
> Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
>
>  include/net/rose.h   | 12 ++++++++++++
>  net/rose/rose_link.c |  5 +++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/net/rose.h b/include/net/rose.h
> index 2b5491bbf39a..33de310ba778 100644
> --- a/include/net/rose.h
> +++ b/include/net/rose.h
> @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
>         }
>  }
>
> +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> +{
> +       struct rose_neigh *rose_neigh = *roseneigh;
> +       if (refcount_dec_and_test(&rose_neigh->use)) {
> +               if (rose_neigh->ax25)
> +                       ax25_cb_put(rose_neigh->ax25);
> +               kfree(rose_neigh->digipeat);
> +               kfree(rose_neigh);
> +               *roseneigh = NULL;
> +       }
> +}

You have not even compiled this patch.

Also please carefully read Documentation/process/maintainer-netdev.rst

Resending after review
~~~~~~~~~~~~~~~~~~~~~~

Allow at least 24 hours to pass between postings. This will ensure reviewers
from all geographical locations have a chance to chime in. Do not wait
too long (weeks) between postings either as it will make it harder for reviewers
to recall all the context.

Make sure you address all the feedback in your new posting. Do not post a new
version of the code if the discussion about the previous version is still
ongoing, unless directly instructed by a reviewer.

The new version of patches should be posted as a separate thread,
not as a reply to the previous posting. Change log should include a link
to the previous posting (see :ref:`Changes requested`).

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Lizhi Xu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 2836 bytes --]

On Fri, 24 Oct 2025 04:49:55 -0700, Eric Dumazet <edumazet@google.com> wrote:
> On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> >
> > There is no synchronization between the two timers, rose_t0timer_expiry
> > and rose_timer_expiry.
> > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > However, rose_t0timer_expiry() does initiate a restart request on the
> > neighbor.
> > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > a UAF is triggered.
> >
> > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > stop restarting t0timer after putting the neighbor.
> >
> > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> >
> > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > Call Trace:
> >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> >
> > Freed by task 17183:
> >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> >  rose_neigh_put include/net/rose.h:165 [inline]
> >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> >
> > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> >
> >  include/net/rose.h   | 12 ++++++++++++
> >  net/rose/rose_link.c |  5 +++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/net/rose.h b/include/net/rose.h
> > index 2b5491bbf39a..33de310ba778 100644
> > --- a/include/net/rose.h
> > +++ b/include/net/rose.h
> > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> >         }
> >  }
> >
> > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > +{
> > +       struct rose_neigh *rose_neigh = *roseneigh;
> > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > +               if (rose_neigh->ax25)
> > +                       ax25_cb_put(rose_neigh->ax25);
> > +               kfree(rose_neigh->digipeat);
> > +               kfree(rose_neigh);
> > +               *roseneigh = NULL;
> > +       }
> > +}
> 
> You have not even compiled this patch.
580K -rw-r--r-- 1 lzx users 578K Oct 24 17:35 net/rose/rose_link.o

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Kuniyuki Iwashima]

On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> There is no synchronization between the two timers, rose_t0timer_expiry
> and rose_timer_expiry.
> rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> However, rose_t0timer_expiry() does initiate a restart request on the
> neighbor.
> When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> a UAF is triggered.
>
> To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> stop restarting t0timer after putting the neighbor.
>
> When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> NULL prevents rose_t0timer_expiry() from restarting t0timer.
>
> syzbot reported a slab-use-after-free Read in ax25_find_cb.
> BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> Call Trace:
>  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
>  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
>  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
>  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
>  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
>
> Freed by task 17183:
>  kfree+0x2b8/0x6d0 mm/slub.c:6826
>  rose_neigh_put include/net/rose.h:165 [inline]
>  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
>
> Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
>
>  include/net/rose.h   | 12 ++++++++++++
>  net/rose/rose_link.c |  5 +++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/include/net/rose.h b/include/net/rose.h
> index 2b5491bbf39a..33de310ba778 100644
> --- a/include/net/rose.h
> +++ b/include/net/rose.h
> @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
>         }
>  }
>
> +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> +{
> +       struct rose_neigh *rose_neigh = *roseneigh;
> +       if (refcount_dec_and_test(&rose_neigh->use)) {
> +               if (rose_neigh->ax25)
> +                       ax25_cb_put(rose_neigh->ax25);
> +               kfree(rose_neigh->digipeat);
> +               kfree(rose_neigh);
> +               *roseneigh = NULL;
> +       }
> +}
> +
>  /* af_rose.c */
>  extern ax25_address rose_callsign;
>  extern int  sysctl_rose_restart_request_timeout;
> diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> index 7746229fdc8c..334c8cc0876d 100644
> --- a/net/rose/rose_link.c
> +++ b/net/rose/rose_link.c
> @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
>
>  static void rose_start_t0timer(struct rose_neigh *neigh)
>  {
> +       if (!neigh)
> +               return;
> +
>         timer_delete(&neigh->t0timer);
>
>         neigh->t0timer.function = rose_t0timer_expiry;
> @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
>  {
>         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
>

What prevents rose_timer_expiry() from releasing the
last refcnt here ?

The t0timer could be triggered even after that happens.


> +       rose_neigh_hold(neigh);
>         rose_transmit_restart_request(neigh);
>
>         neigh->dce_mode = 0;
>
> +       rose_neigh_putex(&neigh);
>         rose_start_t0timer(neigh);
>  }
>
> --
> 2.43.0
>

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Lizhi Xu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 4426 bytes --]

On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> >
> > There is no synchronization between the two timers, rose_t0timer_expiry
> > and rose_timer_expiry.
> > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > However, rose_t0timer_expiry() does initiate a restart request on the
> > neighbor.
> > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > a UAF is triggered.
> >
> > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > stop restarting t0timer after putting the neighbor.
> >
> > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> >
> > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > Call Trace:
> >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> >
> > Freed by task 17183:
> >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> >  rose_neigh_put include/net/rose.h:165 [inline]
> >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> >
> > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> >
> >  include/net/rose.h   | 12 ++++++++++++
> >  net/rose/rose_link.c |  5 +++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/include/net/rose.h b/include/net/rose.h
> > index 2b5491bbf39a..33de310ba778 100644
> > --- a/include/net/rose.h
> > +++ b/include/net/rose.h
> > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> >         }
> >  }
> >
> > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > +{
> > +       struct rose_neigh *rose_neigh = *roseneigh;
> > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > +               if (rose_neigh->ax25)
> > +                       ax25_cb_put(rose_neigh->ax25);
> > +               kfree(rose_neigh->digipeat);
> > +               kfree(rose_neigh);
> > +               *roseneigh = NULL;
> > +       }
> > +}
> > +
> >  /* af_rose.c */
> >  extern ax25_address rose_callsign;
> >  extern int  sysctl_rose_restart_request_timeout;
> > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > index 7746229fdc8c..334c8cc0876d 100644
> > --- a/net/rose/rose_link.c
> > +++ b/net/rose/rose_link.c
> > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> >
> >  static void rose_start_t0timer(struct rose_neigh *neigh)
> >  {
> > +       if (!neigh)
> > +               return;
> > +
> >         timer_delete(&neigh->t0timer);
> >
> >         neigh->t0timer.function = rose_t0timer_expiry;
> > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> >  {
> >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> >
> 
> What prevents rose_timer_expiry() from releasing the
> last refcnt here ?
The issue reported by syzbot is that rose_t0timer_expiry() is triggered
first, followed by rose_timer_expiry().
Therefore, in rose_t0timer_expiry(), the reference count of neigh is
increased before entering rose_transmit_restart_request() to prevent
neigh from being put in rose_timer_expiry(). Then, in rose_t0timer_expiry(),
neigh is put before executing rose_start_t0timer() and the neigh value is
set to NULL to prevent t0timer restarts.

The case where rose_timer_expiry() is triggered before rose_t0timer_expiry()
is not considered at this time.
> 
> The t0timer could be triggered even after that happens.
> 
> 
> > +       rose_neigh_hold(neigh);
> >         rose_transmit_restart_request(neigh);
> >
> >         neigh->dce_mode = 0;
> >
> > +       rose_neigh_putex(&neigh);
> >         rose_start_t0timer(neigh);
> >  }
> >
> > --
> > 2.43.0
> >

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Kuniyuki Iwashima]

On Fri, Oct 24, 2025 at 8:51 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > >
> > > There is no synchronization between the two timers, rose_t0timer_expiry
> > > and rose_timer_expiry.
> > > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > > However, rose_t0timer_expiry() does initiate a restart request on the
> > > neighbor.
> > > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > > a UAF is triggered.
> > >
> > > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > > stop restarting t0timer after putting the neighbor.
> > >
> > > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> > >
> > > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > > Call Trace:
> > >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> > >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> > >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> > >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> > >
> > > Freed by task 17183:
> > >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> > >  rose_neigh_put include/net/rose.h:165 [inline]
> > >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> > >
> > > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> > >
> > >  include/net/rose.h   | 12 ++++++++++++
> > >  net/rose/rose_link.c |  5 +++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/net/rose.h b/include/net/rose.h
> > > index 2b5491bbf39a..33de310ba778 100644
> > > --- a/include/net/rose.h
> > > +++ b/include/net/rose.h
> > > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> > >         }
> > >  }
> > >
> > > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > > +{
> > > +       struct rose_neigh *rose_neigh = *roseneigh;
> > > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > > +               if (rose_neigh->ax25)
> > > +                       ax25_cb_put(rose_neigh->ax25);
> > > +               kfree(rose_neigh->digipeat);
> > > +               kfree(rose_neigh);
> > > +               *roseneigh = NULL;
> > > +       }
> > > +}
> > > +
> > >  /* af_rose.c */
> > >  extern ax25_address rose_callsign;
> > >  extern int  sysctl_rose_restart_request_timeout;
> > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > index 7746229fdc8c..334c8cc0876d 100644
> > > --- a/net/rose/rose_link.c
> > > +++ b/net/rose/rose_link.c
> > > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> > >
> > >  static void rose_start_t0timer(struct rose_neigh *neigh)
> > >  {
> > > +       if (!neigh)
> > > +               return;
> > > +
> > >         timer_delete(&neigh->t0timer);
> > >
> > >         neigh->t0timer.function = rose_t0timer_expiry;
> > > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> > >  {
> > >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> > >
> >
> > What prevents rose_timer_expiry() from releasing the
> > last refcnt here ?
> The issue reported by syzbot is that rose_t0timer_expiry() is triggered
> first, followed by rose_timer_expiry().

I don't see how you read that ordering from the report.
https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d

The only ordering I can find is that kfree() in rose_timer_expiry()
happened before ax25_find_cb () in rose_t0timer_expiry().

> Therefore, in rose_t0timer_expiry(), the reference count of neigh is
> increased before entering rose_transmit_restart_request() to prevent
> neigh from being put in rose_timer_expiry(). Then, in rose_t0timer_expiry(),
> neigh is put before executing rose_start_t0timer() and the neigh value is
> set to NULL to prevent t0timer restarts.
>
> The case where rose_timer_expiry() is triggered before rose_t0timer_expiry()
> is not considered at this time.

So this change just papers over the root cause.


> >
> > The t0timer could be triggered even after that happens.
> >
> >
> > > +       rose_neigh_hold(neigh);
> > >         rose_transmit_restart_request(neigh);
> > >
> > >         neigh->dce_mode = 0;
> > >
> > > +       rose_neigh_putex(&neigh);
> > >         rose_start_t0timer(neigh);
> > >  }
> > >
> > > --
> > > 2.43.0
> > >

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Lizhi Xu]

On Fri, 24 Oct 2025 21:25:20 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> On Fri, Oct 24, 2025 at 8:51 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> >
> > On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > >
> > > > There is no synchronization between the two timers, rose_t0timer_expiry
> > > > and rose_timer_expiry.
> > > > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > > > However, rose_t0timer_expiry() does initiate a restart request on the
> > > > neighbor.
> > > > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > > > a UAF is triggered.
> > > >
> > > > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > > > stop restarting t0timer after putting the neighbor.
> > > >
> > > > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > > > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> > > >
> > > > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > > > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > > > Call Trace:
> > > >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> > > >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> > > >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> > > >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> > > >
> > > > Freed by task 17183:
> > > >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> > > >  rose_neigh_put include/net/rose.h:165 [inline]
> > > >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> > > >
> > > > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > > > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > ---
> > > > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > > > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> > > >
> > > >  include/net/rose.h   | 12 ++++++++++++
> > > >  net/rose/rose_link.c |  5 +++++
> > > >  2 files changed, 17 insertions(+)
> > > >
> > > > diff --git a/include/net/rose.h b/include/net/rose.h
> > > > index 2b5491bbf39a..33de310ba778 100644
> > > > --- a/include/net/rose.h
> > > > +++ b/include/net/rose.h
> > > > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> > > >         }
> > > >  }
> > > >
> > > > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > > > +{
> > > > +       struct rose_neigh *rose_neigh = *roseneigh;
> > > > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > > > +               if (rose_neigh->ax25)
> > > > +                       ax25_cb_put(rose_neigh->ax25);
> > > > +               kfree(rose_neigh->digipeat);
> > > > +               kfree(rose_neigh);
> > > > +               *roseneigh = NULL;
> > > > +       }
> > > > +}
> > > > +
> > > >  /* af_rose.c */
> > > >  extern ax25_address rose_callsign;
> > > >  extern int  sysctl_rose_restart_request_timeout;
> > > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > > index 7746229fdc8c..334c8cc0876d 100644
> > > > --- a/net/rose/rose_link.c
> > > > +++ b/net/rose/rose_link.c
> > > > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> > > >
> > > >  static void rose_start_t0timer(struct rose_neigh *neigh)
> > > >  {
> > > > +       if (!neigh)
> > > > +               return;
> > > > +
> > > >         timer_delete(&neigh->t0timer);
> > > >
> > > >         neigh->t0timer.function = rose_t0timer_expiry;
> > > > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> > > >  {
> > > >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> > > >
> > >
> > > What prevents rose_timer_expiry() from releasing the
> > > last refcnt here ?
> > The issue reported by syzbot is that rose_t0timer_expiry() is triggered
> > first, followed by rose_timer_expiry().
> 
> I don't see how you read that ordering from the report.
> https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
Here's my understanding: See the two calltraces below.
[1] Line 111 occurs after rose_neigh_put(). Otherwise, accessing
neigh->digipeat would result in a UAF. Therefore, rose_t0timer_expiry()
must be triggered before rose_timer_expiry().

[2] syzbot reports that line 237 generates a UAF when accessing digi->ndigi.

UAF Task1:
rose_t0timer_expiry()->
  rose_transmit_restart_request()->
    rose_send_frame(.., neigh->digipeat, ..)-> // [1] line 111
      ax25_find_cb()->
        if (digi != NULL && digi->ndigi != 0)  // [2] line 237

Freed neigh Task2:
 rose_timer_expiry()->
   rose_neigh_put(neigh)->
     kfree(neigh)
> 
> The only ordering I can find is that kfree() in rose_timer_expiry()
> happened before ax25_find_cb () in rose_t0timer_expiry().
> 
> > Therefore, in rose_t0timer_expiry(), the reference count of neigh is
> > increased before entering rose_transmit_restart_request() to prevent
> > neigh from being put in rose_timer_expiry(). Then, in rose_t0timer_expiry(),
> > neigh is put before executing rose_start_t0timer() and the neigh value is
> > set to NULL to prevent t0timer restarts.
> >
> > The case where rose_timer_expiry() is triggered before rose_t0timer_expiry()
> > is not considered at this time.
> 
> So this change just papers over the root cause.
> 
> 
> > >
> > > The t0timer could be triggered even after that happens.
> > >
> > >
> > > > +       rose_neigh_hold(neigh);
> > > >         rose_transmit_restart_request(neigh);
> > > >
> > > >         neigh->dce_mode = 0;
> > > >
> > > > +       rose_neigh_putex(&neigh);
> > > >         rose_start_t0timer(neigh);
> > > >  }
> > > >
> > > > --
> > > > 2.43.0
> > > >

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Kuniyuki Iwashima]

On Fri, Oct 24, 2025 at 11:46 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> On Fri, 24 Oct 2025 21:25:20 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > On Fri, Oct 24, 2025 at 8:51 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > >
> > > On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > > On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > > >
> > > > > There is no synchronization between the two timers, rose_t0timer_expiry
> > > > > and rose_timer_expiry.
> > > > > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > > > > However, rose_t0timer_expiry() does initiate a restart request on the
> > > > > neighbor.
> > > > > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > > > > a UAF is triggered.
> > > > >
> > > > > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > > > > stop restarting t0timer after putting the neighbor.
> > > > >
> > > > > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > > > > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> > > > >
> > > > > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > > > > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > > > > Call Trace:
> > > > >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> > > > >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> > > > >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> > > > >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> > > > >
> > > > > Freed by task 17183:
> > > > >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> > > > >  rose_neigh_put include/net/rose.h:165 [inline]
> > > > >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> > > > >
> > > > > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > > > > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > > ---
> > > > > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > > > > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> > > > >
> > > > >  include/net/rose.h   | 12 ++++++++++++
> > > > >  net/rose/rose_link.c |  5 +++++
> > > > >  2 files changed, 17 insertions(+)
> > > > >
> > > > > diff --git a/include/net/rose.h b/include/net/rose.h
> > > > > index 2b5491bbf39a..33de310ba778 100644
> > > > > --- a/include/net/rose.h
> > > > > +++ b/include/net/rose.h
> > > > > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> > > > >         }
> > > > >  }
> > > > >
> > > > > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > > > > +{
> > > > > +       struct rose_neigh *rose_neigh = *roseneigh;
> > > > > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > > > > +               if (rose_neigh->ax25)
> > > > > +                       ax25_cb_put(rose_neigh->ax25);
> > > > > +               kfree(rose_neigh->digipeat);
> > > > > +               kfree(rose_neigh);
> > > > > +               *roseneigh = NULL;
> > > > > +       }
> > > > > +}
> > > > > +
> > > > >  /* af_rose.c */
> > > > >  extern ax25_address rose_callsign;
> > > > >  extern int  sysctl_rose_restart_request_timeout;
> > > > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > > > index 7746229fdc8c..334c8cc0876d 100644
> > > > > --- a/net/rose/rose_link.c
> > > > > +++ b/net/rose/rose_link.c
> > > > > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> > > > >
> > > > >  static void rose_start_t0timer(struct rose_neigh *neigh)
> > > > >  {
> > > > > +       if (!neigh)
> > > > > +               return;
> > > > > +
> > > > >         timer_delete(&neigh->t0timer);
> > > > >
> > > > >         neigh->t0timer.function = rose_t0timer_expiry;
> > > > > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> > > > >  {
> > > > >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> > > > >
> > > >
> > > > What prevents rose_timer_expiry() from releasing the
> > > > last refcnt here ?
> > > The issue reported by syzbot is that rose_t0timer_expiry() is triggered
> > > first, followed by rose_timer_expiry().
> >
> > I don't see how you read that ordering from the report.
> > https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
> Here's my understanding: See the two calltraces below.

The same question still applies.

What prevents rose_timer_expiry() from releasing the last
refcnt before [1] ?

For example, why is accessing neigh->dev in rose_send_frame()
safe then ?

The commit message mentions that two timers are not
synchronised, but the diff adds no such synchronisation.


> [1] Line 111 occurs after rose_neigh_put(). Otherwise, accessing
> neigh->digipeat would result in a UAF. Therefore, rose_t0timer_expiry()
> must be triggered before rose_timer_expiry().
>
> [2] syzbot reports that line 237 generates a UAF when accessing digi->ndigi.
>
> UAF Task1:
> rose_t0timer_expiry()->
>   rose_transmit_restart_request()->
>     rose_send_frame(.., neigh->digipeat, ..)-> // [1] line 111
>       ax25_find_cb()->
>         if (digi != NULL && digi->ndigi != 0)  // [2] line 237
>
> Freed neigh Task2:
>  rose_timer_expiry()->
>    rose_neigh_put(neigh)->
>      kfree(neigh)
> >
> > The only ordering I can find is that kfree() in rose_timer_expiry()
> > happened before ax25_find_cb () in rose_t0timer_expiry().
> >
> > > Therefore, in rose_t0timer_expiry(), the reference count of neigh is
> > > increased before entering rose_transmit_restart_request() to prevent
> > > neigh from being put in rose_timer_expiry(). Then, in rose_t0timer_expiry(),
> > > neigh is put before executing rose_start_t0timer() and the neigh value is
> > > set to NULL to prevent t0timer restarts.
> > >
> > > The case where rose_timer_expiry() is triggered before rose_t0timer_expiry()
> > > is not considered at this time.
> >
> > So this change just papers over the root cause.
> >
> >
> > > >
> > > > The t0timer could be triggered even after that happens.
> > > >
> > > >
> > > > > +       rose_neigh_hold(neigh);
> > > > >         rose_transmit_restart_request(neigh);
> > > > >
> > > > >         neigh->dce_mode = 0;
> > > > >
> > > > > +       rose_neigh_putex(&neigh);
> > > > >         rose_start_t0timer(neigh);
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.43.0
> > > > >

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Lizhi Xu]

On Sat, 25 Oct 2025 00:15:51 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> On Fri, Oct 24, 2025 at 11:46 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> >
> > On Fri, 24 Oct 2025 21:25:20 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > On Fri, Oct 24, 2025 at 8:51 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > >
> > > > On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > > > On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > > > >
> > > > > > There is no synchronization between the two timers, rose_t0timer_expiry
> > > > > > and rose_timer_expiry.
> > > > > > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > > > > > However, rose_t0timer_expiry() does initiate a restart request on the
> > > > > > neighbor.
> > > > > > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > > > > > a UAF is triggered.
> > > > > >
> > > > > > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > > > > > stop restarting t0timer after putting the neighbor.
> > > > > >
> > > > > > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > > > > > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> > > > > >
> > > > > > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > > > > > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > > > > > Call Trace:
> > > > > >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> > > > > >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> > > > > >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> > > > > >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> > > > > >
> > > > > > Freed by task 17183:
> > > > > >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> > > > > >  rose_neigh_put include/net/rose.h:165 [inline]
> > > > > >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> > > > > >
> > > > > > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > > > > > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > > > ---
> > > > > > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > > > > > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> > > > > >
> > > > > >  include/net/rose.h   | 12 ++++++++++++
> > > > > >  net/rose/rose_link.c |  5 +++++
> > > > > >  2 files changed, 17 insertions(+)
> > > > > >
> > > > > > diff --git a/include/net/rose.h b/include/net/rose.h
> > > > > > index 2b5491bbf39a..33de310ba778 100644
> > > > > > --- a/include/net/rose.h
> > > > > > +++ b/include/net/rose.h
> > > > > > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> > > > > >         }
> > > > > >  }
> > > > > >
> > > > > > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > > > > > +{
> > > > > > +       struct rose_neigh *rose_neigh = *roseneigh;
> > > > > > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > > > > > +               if (rose_neigh->ax25)
> > > > > > +                       ax25_cb_put(rose_neigh->ax25);
> > > > > > +               kfree(rose_neigh->digipeat);
> > > > > > +               kfree(rose_neigh);
> > > > > > +               *roseneigh = NULL;
> > > > > > +       }
> > > > > > +}
> > > > > > +
> > > > > >  /* af_rose.c */
> > > > > >  extern ax25_address rose_callsign;
> > > > > >  extern int  sysctl_rose_restart_request_timeout;
> > > > > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > > > > index 7746229fdc8c..334c8cc0876d 100644
> > > > > > --- a/net/rose/rose_link.c
> > > > > > +++ b/net/rose/rose_link.c
> > > > > > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> > > > > >
> > > > > >  static void rose_start_t0timer(struct rose_neigh *neigh)
> > > > > >  {
> > > > > > +       if (!neigh)
> > > > > > +               return;
> > > > > > +
> > > > > >         timer_delete(&neigh->t0timer);
> > > > > >
> > > > > >         neigh->t0timer.function = rose_t0timer_expiry;
> > > > > > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> > > > > >  {
> > > > > >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> > > > > >
> > > > >
> > > > > What prevents rose_timer_expiry() from releasing the
> > > > > last refcnt here ?
> > > > The issue reported by syzbot is that rose_t0timer_expiry() is triggered
> > > > first, followed by rose_timer_expiry().
> > >
> > > I don't see how you read that ordering from the report.
> > > https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
> > Here's my understanding: See the two calltraces below.
> 
> The same question still applies.
> 
> What prevents rose_timer_expiry() from releasing the last
> refcnt before [1] ?
@@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
 {
 	struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);

+	rose_neigh_hold(neigh); // [3] This prevents rose_timer_expiry() from putting neigh.
 	rose_transmit_restart_request(neigh);

 	neigh->dce_mode = 0;

+	rose_neigh_putex(&neigh); // [4] This prevents t0timer from restarting by setting neigh to NULL.
 	rose_start_t0timer(neigh);
 }
> 
> For example, why is accessing neigh->dev in rose_send_frame()
> safe then ?
> 
> The commit message mentions that two timers are not
> synchronised, but the diff adds no such synchronisation.
> 
> 
> > [1] Line 111 occurs after rose_neigh_put(). Otherwise, accessing
> > neigh->digipeat would result in a UAF. Therefore, rose_t0timer_expiry()
> > must be triggered before rose_timer_expiry().
> >
> > [2] syzbot reports that line 237 generates a UAF when accessing digi->ndigi.
> >
> > UAF Task1:
> > rose_t0timer_expiry()->
> >   rose_transmit_restart_request()->
> >     rose_send_frame(.., neigh->digipeat, ..)-> // [1] line 111
> >       ax25_find_cb()->
> >         if (digi != NULL && digi->ndigi != 0)  // [2] line 237
> >
> > Freed neigh Task2:
> >  rose_timer_expiry()->
> >    rose_neigh_put(neigh)->
> >      kfree(neigh)
> > >
> > > The only ordering I can find is that kfree() in rose_timer_expiry()
> > > happened before ax25_find_cb () in rose_t0timer_expiry().
> > >
> > > > Therefore, in rose_t0timer_expiry(), the reference count of neigh is
> > > > increased before entering rose_transmit_restart_request() to prevent
> > > > neigh from being put in rose_timer_expiry(). Then, in rose_t0timer_expiry(),
> > > > neigh is put before executing rose_start_t0timer() and the neigh value is
> > > > set to NULL to prevent t0timer restarts.
> > > >
> > > > The case where rose_timer_expiry() is triggered before rose_t0timer_expiry()
> > > > is not considered at this time.
> > >
> > > So this change just papers over the root cause.
> > >
> > >
> > > > >
> > > > > The t0timer could be triggered even after that happens.
> > > > >
> > > > >
> > > > > > +       rose_neigh_hold(neigh);
> > > > > >         rose_transmit_restart_request(neigh);
> > > > > >
> > > > > >         neigh->dce_mode = 0;
> > > > > >
> > > > > > +       rose_neigh_putex(&neigh);
> > > > > >         rose_start_t0timer(neigh);
> > > > > >  }
> > > > > >
> > > > > > --
> > > > > > 2.43.0
> > > > > >

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Kuniyuki Iwashima]

On Sat, Oct 25, 2025 at 12:53 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> On Sat, 25 Oct 2025 00:15:51 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > On Fri, Oct 24, 2025 at 11:46 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > >
> > > On Fri, 24 Oct 2025 21:25:20 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > > On Fri, Oct 24, 2025 at 8:51 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > > >
> > > > > On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > > > > On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > > > > >
> > > > > > > There is no synchronization between the two timers, rose_t0timer_expiry
> > > > > > > and rose_timer_expiry.
> > > > > > > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > > > > > > However, rose_t0timer_expiry() does initiate a restart request on the
> > > > > > > neighbor.
> > > > > > > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > > > > > > a UAF is triggered.
> > > > > > >
> > > > > > > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > > > > > > stop restarting t0timer after putting the neighbor.
> > > > > > >
> > > > > > > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > > > > > > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> > > > > > >
> > > > > > > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > > > > > > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > > > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > > > > > > Call Trace:
> > > > > > >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > > >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> > > > > > >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> > > > > > >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> > > > > > >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> > > > > > >
> > > > > > > Freed by task 17183:
> > > > > > >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> > > > > > >  rose_neigh_put include/net/rose.h:165 [inline]
> > > > > > >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> > > > > > >
> > > > > > > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > > > > > > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > > > > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > > > > ---
> > > > > > > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > > > > > > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> > > > > > >
> > > > > > >  include/net/rose.h   | 12 ++++++++++++
> > > > > > >  net/rose/rose_link.c |  5 +++++
> > > > > > >  2 files changed, 17 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/net/rose.h b/include/net/rose.h
> > > > > > > index 2b5491bbf39a..33de310ba778 100644
> > > > > > > --- a/include/net/rose.h
> > > > > > > +++ b/include/net/rose.h
> > > > > > > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> > > > > > >         }
> > > > > > >  }
> > > > > > >
> > > > > > > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > > > > > > +{
> > > > > > > +       struct rose_neigh *rose_neigh = *roseneigh;
> > > > > > > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > > > > > > +               if (rose_neigh->ax25)
> > > > > > > +                       ax25_cb_put(rose_neigh->ax25);
> > > > > > > +               kfree(rose_neigh->digipeat);
> > > > > > > +               kfree(rose_neigh);
> > > > > > > +               *roseneigh = NULL;
> > > > > > > +       }
> > > > > > > +}
> > > > > > > +
> > > > > > >  /* af_rose.c */
> > > > > > >  extern ax25_address rose_callsign;
> > > > > > >  extern int  sysctl_rose_restart_request_timeout;
> > > > > > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > > > > > index 7746229fdc8c..334c8cc0876d 100644
> > > > > > > --- a/net/rose/rose_link.c
> > > > > > > +++ b/net/rose/rose_link.c
> > > > > > > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> > > > > > >
> > > > > > >  static void rose_start_t0timer(struct rose_neigh *neigh)
> > > > > > >  {
> > > > > > > +       if (!neigh)
> > > > > > > +               return;
> > > > > > > +
> > > > > > >         timer_delete(&neigh->t0timer);
> > > > > > >
> > > > > > >         neigh->t0timer.function = rose_t0timer_expiry;
> > > > > > > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> > > > > > >  {
> > > > > > >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> > > > > > >
> > > > > >
> > > > > > What prevents rose_timer_expiry() from releasing the
> > > > > > last refcnt here ?
> > > > > The issue reported by syzbot is that rose_t0timer_expiry() is triggered
> > > > > first, followed by rose_timer_expiry().
> > > >
> > > > I don't see how you read that ordering from the report.
> > > > https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
> > > Here's my understanding: See the two calltraces below.
> >
> > The same question still applies.
> >
> > What prevents rose_timer_expiry() from releasing the last
> > refcnt before [1] ?
> @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
>  {
>         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
>
> +       rose_neigh_hold(neigh); // [3] This prevents rose_timer_expiry() from putting neigh.

If you ask yourself the same question once more here,
you will notice the fix is broken.

What prevents rose_timer_expiry() from releasing the
last refcnt before rose_neigh_hold() ?

Do you add another rose_neigh_hold() before
rose_neigh_hold() ?

... and the same question applies as long as you are
trying to fix the bug by adding changes in rose_t0timer_expiry().


>         rose_transmit_restart_request(neigh);
>
>         neigh->dce_mode = 0;
>
> +       rose_neigh_putex(&neigh); // [4] This prevents t0timer from restarting by setting neigh to NULL.
>         rose_start_t0timer(neigh);
>  }
> >
> > For example, why is accessing neigh->dev in rose_send_frame()
> > safe then ?
> >
> > The commit message mentions that two timers are not
> > synchronised, but the diff adds no such synchronisation.
> >
> >
> > > [1] Line 111 occurs after rose_neigh_put(). Otherwise, accessing
> > > neigh->digipeat would result in a UAF. Therefore, rose_t0timer_expiry()
> > > must be triggered before rose_timer_expiry().
> > >
> > > [2] syzbot reports that line 237 generates a UAF when accessing digi->ndigi.
> > >
> > > UAF Task1:
> > > rose_t0timer_expiry()->
> > >   rose_transmit_restart_request()->
> > >     rose_send_frame(.., neigh->digipeat, ..)-> // [1] line 111
> > >       ax25_find_cb()->
> > >         if (digi != NULL && digi->ndigi != 0)  // [2] line 237
> > >
> > > Freed neigh Task2:
> > >  rose_timer_expiry()->
> > >    rose_neigh_put(neigh)->
> > >      kfree(neigh)
> > > >
> > > > The only ordering I can find is that kfree() in rose_timer_expiry()
> > > > happened before ax25_find_cb () in rose_t0timer_expiry().
> > > >
> > > > > Therefore, in rose_t0timer_expiry(), the reference count of neigh is
> > > > > increased before entering rose_transmit_restart_request() to prevent
> > > > > neigh from being put in rose_timer_expiry(). Then, in rose_t0timer_expiry(),
> > > > > neigh is put before executing rose_start_t0timer() and the neigh value is
> > > > > set to NULL to prevent t0timer restarts.
> > > > >
> > > > > The case where rose_timer_expiry() is triggered before rose_t0timer_expiry()
> > > > > is not considered at this time.
> > > >
> > > > So this change just papers over the root cause.
> > > >
> > > >
> > > > > >
> > > > > > The t0timer could be triggered even after that happens.
> > > > > >
> > > > > >
> > > > > > > +       rose_neigh_hold(neigh);
> > > > > > >         rose_transmit_restart_request(neigh);
> > > > > > >
> > > > > > >         neigh->dce_mode = 0;
> > > > > > >
> > > > > > > +       rose_neigh_putex(&neigh);
> > > > > > >         rose_start_t0timer(neigh);
> > > > > > >  }
> > > > > > >
> > > > > > > --
> > > > > > > 2.43.0
> > > > > > >

Re: [PATCH V3] net: rose: Prevent the use of freed digipeat

[Lizhi Xu]

On Mon, 27 Oct 2025 10:40:34 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> On Sat, Oct 25, 2025 at 12:53 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> >
> > On Sat, 25 Oct 2025 00:15:51 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > On Fri, Oct 24, 2025 at 11:46 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > >
> > > > On Fri, 24 Oct 2025 21:25:20 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > > > On Fri, Oct 24, 2025 at 8:51 PM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > > > >
> > > > > > On Fri, 24 Oct 2025 19:18:46 -0700, Kuniyuki Iwashima <kuniyu@google.com> wrote:
> > > > > > > On Fri, Oct 24, 2025 at 2:39 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
> > > > > > > >
> > > > > > > > There is no synchronization between the two timers, rose_t0timer_expiry
> > > > > > > > and rose_timer_expiry.
> > > > > > > > rose_timer_expiry() puts the neighbor when the rose state is ROSE_STATE_2.
> > > > > > > > However, rose_t0timer_expiry() does initiate a restart request on the
> > > > > > > > neighbor.
> > > > > > > > When rose_t0timer_expiry() accesses the released neighbor member digipeat,
> > > > > > > > a UAF is triggered.
> > > > > > > >
> > > > > > > > To avoid this UAF, defer the put operation to rose_t0timer_expiry() and
> > > > > > > > stop restarting t0timer after putting the neighbor.
> > > > > > > >
> > > > > > > > When putting the neighbor, set the neighbor to NULL. Setting neighbor to
> > > > > > > > NULL prevents rose_t0timer_expiry() from restarting t0timer.
> > > > > > > >
> > > > > > > > syzbot reported a slab-use-after-free Read in ax25_find_cb.
> > > > > > > > BUG: KASAN: slab-use-after-free in ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > > > > Read of size 1 at addr ffff888059c704c0 by task syz.6.2733/17200
> > > > > > > > Call Trace:
> > > > > > > >  ax25_find_cb+0x3b8/0x3f0 net/ax25/af_ax25.c:237
> > > > > > > >  ax25_send_frame+0x157/0xb60 net/ax25/ax25_out.c:55
> > > > > > > >  rose_send_frame+0xcc/0x2c0 net/rose/rose_link.c:106
> > > > > > > >  rose_transmit_restart_request+0x1b8/0x240 net/rose/rose_link.c:198
> > > > > > > >  rose_t0timer_expiry+0x1d/0x150 net/rose/rose_link.c:83
> > > > > > > >
> > > > > > > > Freed by task 17183:
> > > > > > > >  kfree+0x2b8/0x6d0 mm/slub.c:6826
> > > > > > > >  rose_neigh_put include/net/rose.h:165 [inline]
> > > > > > > >  rose_timer_expiry+0x537/0x630 net/rose/rose_timer.c:183
> > > > > > > >
> > > > > > > > Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
> > > > > > > > Reported-by: syzbot+caa052a0958a9146870d@syzkaller.appspotmail.com
> > > > > > > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > > > > > > ---
> > > > > > > > V1 -> V2: Putting the neighbor stops t0timer from automatically starting
> > > > > > > > V2 -> V3: add rose_neigh_putex for set rose neigh to NULL
> > > > > > > >
> > > > > > > >  include/net/rose.h   | 12 ++++++++++++
> > > > > > > >  net/rose/rose_link.c |  5 +++++
> > > > > > > >  2 files changed, 17 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/net/rose.h b/include/net/rose.h
> > > > > > > > index 2b5491bbf39a..33de310ba778 100644
> > > > > > > > --- a/include/net/rose.h
> > > > > > > > +++ b/include/net/rose.h
> > > > > > > > @@ -167,6 +167,18 @@ static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
> > > > > > > >         }
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static inline void rose_neigh_putex(struct rose_neigh **roseneigh)
> > > > > > > > +{
> > > > > > > > +       struct rose_neigh *rose_neigh = *roseneigh;
> > > > > > > > +       if (refcount_dec_and_test(&rose_neigh->use)) {
> > > > > > > > +               if (rose_neigh->ax25)
> > > > > > > > +                       ax25_cb_put(rose_neigh->ax25);
> > > > > > > > +               kfree(rose_neigh->digipeat);
> > > > > > > > +               kfree(rose_neigh);
> > > > > > > > +               *roseneigh = NULL;
> > > > > > > > +       }
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /* af_rose.c */
> > > > > > > >  extern ax25_address rose_callsign;
> > > > > > > >  extern int  sysctl_rose_restart_request_timeout;
> > > > > > > > diff --git a/net/rose/rose_link.c b/net/rose/rose_link.c
> > > > > > > > index 7746229fdc8c..334c8cc0876d 100644
> > > > > > > > --- a/net/rose/rose_link.c
> > > > > > > > +++ b/net/rose/rose_link.c
> > > > > > > > @@ -43,6 +43,9 @@ void rose_start_ftimer(struct rose_neigh *neigh)
> > > > > > > >
> > > > > > > >  static void rose_start_t0timer(struct rose_neigh *neigh)
> > > > > > > >  {
> > > > > > > > +       if (!neigh)
> > > > > > > > +               return;
> > > > > > > > +
> > > > > > > >         timer_delete(&neigh->t0timer);
> > > > > > > >
> > > > > > > >         neigh->t0timer.function = rose_t0timer_expiry;
> > > > > > > > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> > > > > > > >  {
> > > > > > > >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> > > > > > > >
> > > > > > >
> > > > > > > What prevents rose_timer_expiry() from releasing the
> > > > > > > last refcnt here ?
> > > > > > The issue reported by syzbot is that rose_t0timer_expiry() is triggered
> > > > > > first, followed by rose_timer_expiry().
> > > > >
> > > > > I don't see how you read that ordering from the report.
> > > > > https://syzkaller.appspot.com/bug?extid=caa052a0958a9146870d
> > > > Here's my understanding: See the two calltraces below.
> > >
> > > The same question still applies.
> > >
> > > What prevents rose_timer_expiry() from releasing the last
> > > refcnt before [1] ?
> > @@ -80,10 +83,12 @@ static void rose_t0timer_expiry(struct timer_list *t)
> >  {
> >         struct rose_neigh *neigh = timer_container_of(neigh, t, t0timer);
> >
> > +       rose_neigh_hold(neigh); // [3] This prevents rose_timer_expiry() from putting neigh.
> 
> If you ask yourself the same question once more here,
> you will notice the fix is broken.
> 
> What prevents rose_timer_expiry() from releasing the
> last refcnt before rose_neigh_hold() ?
The UAF issue reported by syzbot is shown below:
	CPU0				CPU1
	====				====
 rose_t0timer_expiry()
 rose_transmit_restart_request()
 rose_send_frame()
 ax25_send_frame()			rose_timer_expiry()
 					rose_neigh_put()
					kfree(neigh)
 ax25_find_cb()

My patch calls rose_neigh_hold() before executing rose_transmit_restart_request()
in rose_t0timer_expiry(). It then calls rose_neigh_putex() to release and
set neigh to NULL before executing rose_start_t0timer(). This also prevents
timer0 from restarting.

I think the only questionable part of the patch is the expiration time of
rose_timer. I don't know the expiration time because I don't have a reproducer.
If the value is very small, the result may be different.