NetBSD Problem Report #58201

From mdehling@gmail.com  Fri Apr 26 17:32:47 2024
Return-Path: <mdehling@gmail.com>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id AB4661A9238
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 26 Apr 2024 17:32:47 +0000 (UTC)
Message-Id: <CAP+FTfUu7TauzQifLsqEiv7wC8zAED4kZOwpuC9aGvNH2YYLFA@mail.gmail.com>
Date: Fri, 26 Apr 2024 12:32:45 -0500
From: Malte Dehling <mdehling@gmail.com>
To: NetBSD Bugs <gnats-bugs@netbsd.org>
Subject: acpibat(4) is missing call to acpi_deregister_notify
X-Send-Pr-Version: 3.95

>Number:         58201
>Category:       kern
>Synopsis:       acpibat(4) is missing call to acpi_deregister_notify
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Apr 26 17:35:00 +0000 2024
>Closed-Date:    Fri Apr 26 19:21:41 +0000 2024
>Last-Modified:  Sat Apr 27 00:40:01 +0000 2024
>Originator:     Malte Dehling
>Release:        NetBSD 10.0, -current
>Organization:
>Environment:
    NetBSD 10.0 (GENERIC) #4: Wed Apr 24 12:21:26 PDT 2024
    mdehling@nb-base-dev:/scratch/obj/sys/arch/amd64/compile/GENERIC amd64

>Description:

Missing call to acpi_deregister_notify leads to use of resources that
have been freed in the (unlikely) case the call to
sysmon_envsys_register() fails.

>How-To-Repeat:

>Fix:

Prevent an issue in the unlikely case sysmon_envsys_register() fails.
---
 sys/dev/acpi/acpi_bat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sys/dev/acpi/acpi_bat.c b/sys/dev/acpi/acpi_bat.c
index 617a2666c26..f8aa3925595 100644
--- a/sys/dev/acpi/acpi_bat.c
+++ b/sys/dev/acpi/acpi_bat.c
@@ -776,6 +776,8 @@ acpibat_init_envsys(device_t dv)
 fail:
 	aprint_error_dev(dv, "failed to initialize sysmon\n");

+	(void)acpi_deregister_notify(sc->sc_node);
+
 	sysmon_envsys_destroy(sc->sc_sme);
 	kmem_free(sc->sc_sensor, ACPIBAT_COUNT * sizeof(*sc->sc_sensor));

-- 
Malte Dehling

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58201 CVS commit: src/sys/dev/acpi
Date: Fri, 26 Apr 2024 14:19:18 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Fri Apr 26 18:19:18 UTC 2024

 Modified Files:
 	src/sys/dev/acpi: acpi_bat.c

 Log Message:
 PR/58201: Malte Dehling: re-order sysmon initialization before acpi
 registration, to avoid needing to call to acpi_deregister_notify on sysmon
 failure.


 To generate a diff of this commit:
 cvs rdiff -u -r1.121 -r1.122 src/sys/dev/acpi/acpi_bat.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Fri, 26 Apr 2024 19:21:41 +0000
State-Changed-Why:
Fixed by christos, thanks!



From: Malte Dehling <mdehling@gmail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/58201 CVS commit: src/sys/dev/acpi
Date: Fri, 26 Apr 2024 15:30:39 -0500

 On Fri, Apr 26, 2024 at 06:20:01PM +0000, Christos Zoulas wrote:
 > The following reply was made to PR kern/58201; it has been noted by GNATS.
 >
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc:
 > Subject: PR/58201 CVS commit: src/sys/dev/acpi
 > Date: Fri, 26 Apr 2024 14:19:18 -0400
 >
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Fri Apr 26 18:19:18 UTC 2024
 >
 >  Modified Files:
 >  	src/sys/dev/acpi: acpi_bat.c
 >
 >  Log Message:
 >  PR/58201: Malte Dehling: re-order sysmon initialization before acpi
 >  registration, to avoid needing to call to acpi_deregister_notify on sysmon
 >  failure.
 >
 >
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.121 -r1.122 src/sys/dev/acpi/acpi_bat.c
 >
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >

 The call to sysmon_envsys_register() will trigger an initial refresh and
 the refresh function looks at sc->sc_last which is uninitialized since
 acpibat_update_status() hasn't been called yet.

 -- 
 Malte Dehling

From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 Malte Dehling <mdehling@gmail.com>
Subject: Re: PR/58201 CVS commit: src/sys/dev/acpi
Date: Fri, 26 Apr 2024 17:41:42 -0400

 --Apple-Mail=_251F196E-3EE1-4F64-97BF-03C62896E190
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/plain;
 	charset=utf-8



 > On Apr 26, 2024, at 4:35=E2=80=AFPM, Malte Dehling =
 <mdehling@gmail.com> wrote:
 >=20
 > The following reply was made to PR kern/58201; it has been noted by =
 GNATS.
 >=20
 > The call to sysmon_envsys_register() will trigger an initial refresh =
 and
 > the refresh function looks at sc->sc_last which is uninitialized since
 > acpibat_update_status() hasn't been called yet.
 >=20

 It is zeroed, so it should return immediately, right?
 Perhaps I should add a memset to it since acpi_bat.c
 zero's some of the fields explicitly anyway (but it should not need to).

 https://nxr.netbsd.org/xref/src/sys/kern/subr_autoconf.c#1591

 christos=

 --Apple-Mail=_251F196E-3EE1-4F64-97BF-03C62896E190
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/html;
 	charset=utf-8

 <html><head><meta http-equiv=3D"content-type" content=3D"text/html; =
 charset=3Dutf-8"></head><body style=3D"overflow-wrap: break-word; =
 -webkit-nbsp-mode: space; line-break: after-white-space;"><br =
 id=3D"lineBreakAtBeginningOfMessage"><div><br><blockquote =
 type=3D"cite"><div>On Apr 26, 2024, at 4:35=E2=80=AFPM, Malte Dehling =
 &lt;mdehling@gmail.com&gt; wrote:</div><br =
 class=3D"Apple-interchange-newline"><div><div>The following reply was =
 made to PR kern/58201; it has been noted by GNATS.<br><br> The call to =
 sysmon_envsys_register() will trigger an initial refresh and<br> the =
 refresh function looks at sc-&gt;sc_last which is uninitialized =
 since<br> acpibat_update_status() hasn't been called =
 yet.<br><br></div></div></blockquote><div><br></div>It is zeroed, so it =
 should return immediately, right?</div><div>Perhaps I should add a =
 memset to it since acpi_bat.c</div><div>zero's some of the fields =
 explicitly anyway (but it should not need =
 to).</div><div><br></div><div><a =
 href=3D"https://nxr.netbsd.org/xref/src/sys/kern/subr_autoconf.c#1591">htt=
 ps://nxr.netbsd.org/xref/src/sys/kern/subr_autoconf.c#1591</a><div><br></d=
 iv></div>christos</body></html>=

 --Apple-Mail=_251F196E-3EE1-4F64-97BF-03C62896E190--

From: Malte Dehling <mdehling@gmail.com>
To: GNATS Bugs <gnats-bugs@netbsd.org>
Cc: 
Subject: Re: Re: PR/58201 CVS commit: src/sys/dev/acpi
Date: Fri, 26 Apr 2024 16:39:35 -0700

 --000000000000a0c8f00617086aaa
 Content-Type: text/plain; charset="UTF-8"
 Content-Transfer-Encoding: quoted-printable

 On Fri, Apr 26, 2024 at 05:41:42PM -0400, Christos Zoulas wrote:
 > > On Apr 26, 2024, at 4:35=E2=80=AFPM, Malte Dehling <mdehling@gmail.com>=
  wrote:
 > > The call to sysmon_envsys_register() will trigger an initial refresh an=
 d
 > > the refresh function looks at sc->sc_last which is uninitialized since
 > > acpibat_update_status() hasn't been called yet.
 >
 > It is zeroed, so it should return immediately, right?
 > Perhaps I should add a memset to it since acpi_bat.c
 > zero's some of the fields explicitly anyway (but it should not need to).

 Thanks, you're right!  I didn't realize the softc struct is zeroed.

 So with refresh returning immediately, the initial limits will be 0 and
 updated with the following call to acpibat_update_info.  That seems safe
 to me.  Should we get rid of SME_INIT_REFRESH to avoid the unnecessary
 refresh call?

 --=20
 Malte Dehling

 --000000000000a0c8f00617086aaa
 Content-Type: application/pgp-signature; name="signature.asc"
 Content-Disposition: attachment; filename="signature.asc"
 Content-Transfer-Encoding: base64
 X-Attachment-Id: 4edad17c2d048f0c_0.1

 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUFCQ0FBZEZpRUVva3ZsOUFOMEFo
 T05jQXdlSzhMazZxS3dkMFFGQW1Zc095OEFDZ2tRSzhMazZxS3cKZDBUNFZSQUFySlB4dElzR1Nh
 Nm01WjFOYXdWOExNUjE2ck5td1hpR2JFSmg2SmJLTUZqUzVLRU1NNk4za1hybApVa2w5Wkl3YWRj
 a3JQK0VFRFN1dFNEZ2xWVC9YTVI4bW40WGFrcFU1V2wwS09kM1FtSXR5anVuMW9CbDkrcHBxClgw
 djNselpRY2lMWDJNNFoxVUx3NWRnQk5kTzRmK0ZaZW9SOGN1QlByeUVKNWtsRHVjbm13d21MNk1F
 TDc1bVYKMzBGZUdRU0dxb09ENGhlNEFlcXZlTDZQUFlTRGVuZDQrckZmOEo1aDRrZFFYMWx3Z3l5
 R3lFZFVXUE9WNVh4RwptQklyNHRsSCt1WThpTFdyMERUVEV2enNVZEc4YTZhcHVVUkRhNUVnbmNw
 UzNhZWh1R0VYdWlpR0dkQXdXdHpUCk51RFA4amtMczVYV0svUGJnN0c1VTZvcDNGV3E0YVROZkcz
 QVVRSGt0RFBDa2F0aUhQK0NLMllvaktUZUIrTEEKUzdpdTVPeU5TSFNEclNVVlp5MTBBdEUwQzlE
 NjRLWExob01RbTkyc25lZ3ZwL3J5cm1EdkN6ZVVKVFBnUzJxZApxU1hmMFd4S2lyVzNHdDhBSjhy
 M2w3akxhci9qUHRmMXZZK0FJbkRNRWVHeE52NjNBL2NIMTFJTDhYeEc4N2tsCkI1MGJUMDZjdzFs
 cjV4NnIvZFZpeTRXeWJKTUlRSnZobjNKbytCdnF4RFVhdGV5ejZQaUpXMHlsY2x3UmEzMUkKTWk1
 MW1pUU5hRE5SNUF2MUR5YlRTYlVPRkIzZ1NFMzBMMmc1MG5tVmF1UEIxeGVRY2phU0JWeE9xN2Qx
 SEhZdApFMk9VaEtpRTd4c0FxWFk2eXBQUVNjUFN0WXNJR1cyK2JyYU5LcWx3VUpldUorY2YxQWc9
 Cj0wcWRYCi0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo=
 --000000000000a0c8f00617086aaa--

From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 Malte Dehling <mdehling@gmail.com>
Subject: Re: PR/58201 CVS commit: src/sys/dev/acpi
Date: Fri, 26 Apr 2024 20:35:37 -0400

 Yes, I will do that and remove the other 0's.

 Thanks,

 christos

>Unformatted:

Home	PR Database Search