Re: net/if.c: nullptr deref in if_hooks_run

2020-03-09 Thread Todd C . Miller 
On Tue, 10 Mar 2020 10:25:56 +1000, David Gwynne wrote:

> how about this? this has the cursor handling move the traversal and
> NULL check back to the for loop instead of looping on its own.

Much better.  OK millert@

 - todd

Re: net/if.c: nullptr deref in if_hooks_run

2020-03-09 Thread David Gwynne 
On Mon, Mar 09, 2020 at 11:56:09PM +0100, Klemens Nanni wrote:
> On Mon, Mar 09, 2020 at 10:33:17PM +0100, Tobias Heider wrote:
> > there seems to be a nullptr dereference in if_hooks_run.
> Did your kernel crash here or did you find reading alone?
> 
> > When the inner while loop is exited because 't == NULL' the next
> > line is an access to 't->t_func'.
> Yes, reads obviously wrong.

:'(

> > Because 't==NULL' means the TAILQ is fully traversed I think we
> > should break and exit instead.
> Make sense, OK kn

how about this? this has the cursor handling move the traversal and
NULL check back to the for loop instead of looping on its own.

Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.600
diff -u -p -r1.600 if.c
--- if.c24 Jan 2020 05:14:51 -  1.600
+++ if.c10 Mar 2020 00:24:00 -
@@ -1050,10 +1050,9 @@ if_hooks_run(struct task_list *hooks)
 
mtx_enter(&if_hooks_mtx);
for (t = TAILQ_FIRST(hooks); t != NULL; t = nt) {
-   while (t->t_func == NULL) { /* skip cursors */
-   t = TAILQ_NEXT(t, t_entry);
-   if (t == NULL)
-   break;
+   if (t->t_func == NULL) { /* skip cursors */
+   nt = TAILQ_NEXT(t, t_entry);
+   continue;
}
func = t->t_func;
arg = t->t_arg;

Re: net/if.c: nullptr deref in if_hooks_run

2020-03-09 Thread Tobias Heider 
On Mon, Mar 09, 2020 at 11:56:09PM +0100, Klemens Nanni wrote:
> On Mon, Mar 09, 2020 at 10:33:17PM +0100, Tobias Heider wrote:
> > there seems to be a nullptr dereference in if_hooks_run.
> Did your kernel crash here or did you find reading alone?

Coverity Scan found it

> > When the inner while loop is exited because 't == NULL' the next
> > line is an access to 't->t_func'.
> Yes, reads obviously wrong.
> 
> > Because 't==NULL' means the TAILQ is fully traversed I think we
> > should break and exit instead.
> Make sense, OK kn
>

Re: net/if.c: nullptr deref in if_hooks_run

2020-03-09 Thread Klemens Nanni 
On Mon, Mar 09, 2020 at 10:33:17PM +0100, Tobias Heider wrote:
> there seems to be a nullptr dereference in if_hooks_run.
Did your kernel crash here or did you find reading alone?

> When the inner while loop is exited because 't == NULL' the next
> line is an access to 't->t_func'.
Yes, reads obviously wrong.

> Because 't==NULL' means the TAILQ is fully traversed I think we
> should break and exit instead.
Make sense, OK kn

net/if.c: nullptr deref in if_hooks_run

2020-03-09 Thread Tobias Heider 
Hi,

there seems to be a nullptr dereference in if_hooks_run.
When the inner while loop is exited because 't == NULL' the next
line is an access to 't->t_func'.
Because 't==NULL' means the TAILQ is fully traversed I think we
should break and exit instead.

ok?

Index: if.c
===
RCS file: /mount/openbsd/cvs/src/sys/net/if.c,v
retrieving revision 1.600
diff -u -p -r1.600 if.c
--- if.c24 Jan 2020 05:14:51 -  1.600
+++ if.c9 Mar 2020 21:25:06 -
@@ -1055,6 +1055,8 @@ if_hooks_run(struct task_list *hooks)
if (t == NULL)
break;
}
+   if (t == NULL)
+   break;
func = t->t_func;
arg = t->t_arg;