Created attachment 231297 [details] A simple fix for this bug In mld_is_addr_reported(), the return value of mli_alloc_locked() is assigned to mli and there is a dereference of it after that, which could lead to NULL pointer dereference on failure of allocation. Fix this bug by adding a NULL check of mli. This bug is found by a static analyzer, please advise.
It's mld_domifattach(), right? And NULL check is "if (mli == NULL)".
(In reply to Yuri from comment #1) Yes you're right, the fix is not proper.
A better solution is to ensure that the failure doesn't happen in the first place: https://reviews.freebsd.org/D34943
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5d691ab4f03d436d38f46777c3c117cf5a27f1bc commit 5d691ab4f03d436d38f46777c3c117cf5a27f1bc Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2022-04-21 17:22:09 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2022-04-21 17:23:59 +0000 mld6: Ensure that mld_domifattach() always succeeds mld_domifattach() does a memory allocation under the global MLD mutex and so can fail, but no error handling prevents a null pointer dereference in this case. The mutex is only needed when updating the global softc list; the allocation and static initialization of the softc does not require this mutex. So, reduce the scope of the mutex and use M_WAITOK for the allocation. PR: 261457 MFC after: 1 week Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34943 sys/netinet6/mld6.c | 48 ++++++++++-------------------------------------- 1 file changed, 10 insertions(+), 38 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=46242829baa3c822b8e7376a71f1d0fa2be1c5c5 commit 46242829baa3c822b8e7376a71f1d0fa2be1c5c5 Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2022-04-21 17:22:09 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2022-04-28 00:34:17 +0000 mld6: Ensure that mld_domifattach() always succeeds mld_domifattach() does a memory allocation under the global MLD mutex and so can fail, but no error handling prevents a null pointer dereference in this case. The mutex is only needed when updating the global softc list; the allocation and static initialization of the softc does not require this mutex. So, reduce the scope of the mutex and use M_WAITOK for the allocation. PR: 261457 Sponsored by: The FreeBSD Foundation (cherry picked from commit 5d691ab4f03d436d38f46777c3c117cf5a27f1bc) sys/netinet6/mld6.c | 48 ++++++++++-------------------------------------- 1 file changed, 10 insertions(+), 38 deletions(-)
Thanks for the report.