Bug 275515

Summary:	Out of bounds memory access in siba_bhndb.c
Product:	Base System	Reporter:	Frank Hilgendorf <frank.hilgendorf>
Component:	wireless	Assignee:	Mark Johnston <markj>
Status:	Closed FIXED
Severity:	Affects Only Me	CC:	markj
Priority:	---
Version:	14.0-RELEASE
Hardware:	Any
OS:	Any

Description Frank Hilgendorf 2023-12-03 23:19:21 UTC

In /usr/src/sys/dev/bhnd/siba/siba_bhndb.c, in the class definition a wrong softc struct is used. This causes out of bound memory accesses in the driver. 
These were observed with KASAN activated in the Kernel.

Hardware:
————————-
Macbook Pro 3,1 with Broadcom BCM4321 wireless card


Patch:
—————-
289 - sizeof(struct siba_softc), bhnd_bhndb_driver, siba_driver);
289 + sizeof(struct siba_bhndb_softc), bhnd_bhndb_driver, siba_driver);

Comment 1 Mark Johnston freebsd_committer

2023-12-04 15:03:23 UTC

This looks good to me.  How did you notice the problem?  struct siba_bhndb_softc initializes the "quirks" field (the only out-of-bounds field) to 0 during attach, and I suspect that malloc() size padding would hide the problem.

Comment 2 Frank Hilgendorf 2023-12-05 17:37:04 UTC

I found it while figuring out why the bwn driver causes a kernel panic after connection to my WLAN… So I activated KASAN that then revealed the problem.
Unfortunately, it was not the root cause for the bwn problem. 

I soon will file a PR on that, too.

Comment 3 Mark Johnston freebsd_committer

2023-12-05 18:46:15 UTC

(In reply to Frank Hilgendorf from comment #2)
I see, thank you.  Indeed, I think this bug is "harmless", but will commit a patch today.

Comment 4 commit-hook freebsd_committer

2023-12-05 18:55:53 UTC

A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=4c3aa00c0a0093c78f42d138bb9eef9b1a7cbb39

commit 4c3aa00c0a0093c78f42d138bb9eef9b1a7cbb39
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-12-05 18:47:03 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-12-05 18:47:03 +0000

    bhnd: Correct the softc size in the siba_bhndb_driver definition

    struct siba_bhndb_softc embeds struct siba_softc and adds an extra
    field, "quirks".  In practice, this bug was harmless since "quirks" is
    unconditionally initialized during driver attach and would have lived in
    the redzone of the softc allocation, but KASAN catches the out-of-bounds
    access.

    PR:             275515
    Reported by:    Frank Hilgendorf <frank.hilgendorf@posteo.de>
    MFC after:      1 week

 sys/dev/bhnd/siba/siba_bhndb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comment 5 commit-hook freebsd_committer

2023-12-12 00:31:12 UTC

A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d264ddb9c7f8739be22170ec660110a0f4c2ec8f

commit d264ddb9c7f8739be22170ec660110a0f4c2ec8f
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-12-05 18:47:03 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-12-12 00:30:05 +0000

    bhnd: Correct the softc size in the siba_bhndb_driver definition

    struct siba_bhndb_softc embeds struct siba_softc and adds an extra
    field, "quirks".  In practice, this bug was harmless since "quirks" is
    unconditionally initialized during driver attach and would have lived in
    the redzone of the softc allocation, but KASAN catches the out-of-bounds
    access.

    PR:             275515
    Reported by:    Frank Hilgendorf <frank.hilgendorf@posteo.de>
    MFC after:      1 week

    (cherry picked from commit 4c3aa00c0a0093c78f42d138bb9eef9b1a7cbb39)

 sys/dev/bhnd/siba/siba_bhndb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comment 6 Mark Johnston freebsd_committer

2023-12-12 00:38:31 UTC

Thank you for the report.