NetBSD Problem Report #58663

From www@netbsd.org  Wed Sep  4 04:13:34 2024
Return-Path: <www@netbsd.org>
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)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 6650A1A923B
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  4 Sep 2024 04:13:34 +0000 (UTC)
Message-Id: <20240904041332.CA16D1A923C@mollari.NetBSD.org>
Date: Wed,  4 Sep 2024 04:13:32 +0000 (UTC)
From: lloyd@must-have-coffee.gen.nz
Reply-To: lloyd@must-have-coffee.gen.nz
To: gnats-bugs@NetBSD.org
Subject: gpt(1) biosboot.c doesn't compile on modern Linux
X-Send-Pr-Version: www-1.0

>Number:         58663
>Category:       toolchain
>Synopsis:       gpt(1) biosboot.c doesn't compile on modern Linux
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    lloyd
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 04 04:15:01 +0000 2024
>Closed-Date:    Wed Oct 09 15:05:32 +0000 2024
>Last-Modified:  Wed Oct 09 23:20:01 +0000 2024
>Originator:     Lloyd Parkes
>Release:        10.99.12
>Organization:
Must Have Coffee
>Environment:
Linux riftsweeper.must-have-coffee.gen.nz 6.10.6-arch1-1 #1 SMP PREEMPT_DYNAMIC Mon, 19 Aug 2024 17:02:39 +0000 x86_64 GNU/Linux

>Description:
The function cmd_biosboot() has a variable called start of type daddr_t. It sets this variable by calling gpt_human_get(gpt, &start). 

The problem is that gpt_human_get() expects a pointer to an off_t, not a pointer to a daddr_t and on Linux these two types are different sizes. This will result in memory corruption on Linux as gpt_human_get() will modify memory that cmd_biosboot() doesn't expect it to. 

An easy fix is to change the type of variable start to off_t. The later uses of start will work out fine because C implicit type conversion handles things for us.
>How-To-Repeat:
Build on amd64 Arch Linux which is new enough to have GCC 14.
>Fix:
Apply the following patch.
diff -r 3a2c38eb3772 sbin/gpt/biosboot.c
--- a/sbin/gpt/biosboot.c       Tue Sep 03 19:51:02 2024 +0000
+++ b/sbin/gpt/biosboot.c       Wed Sep 04 15:08:01 2024 +1200
@@ -267,7 +267,7 @@
 #endif
        int ch;
        gpt_t ngpt = gpt;
-       daddr_t start = 0;
+       off_t start = 0;        /* off_t because of gpt_human_get() */
        uint64_t size = 0;
        int active = 0;
        unsigned int entry = 0;

>Release-Note:

>Audit-Trail:
From: matthew green <mrg@eterna23.net>
To: gnats-bugs@netbsd.org
Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: re: toolchain/58663: gpt(1) biosboot.c doesn't compile on modern Linux
Date: Wed, 04 Sep 2024 14:33:04 +1000

 > -       daddr_t start = 0;
 > +       off_t start = 0;        /* off_t because of gpt_human_get() */

 i think your change is generally correct, but it really indicates
 it much larger problem.

 daddr_t should be made to be 64-bit. (except for the cases where netbsd
 is building 32-bit daddr_t values for eg, old platform boot programs.)

 ie, this seems like it is *also* a compat issue we should fix, it may
 be causing weird issues elsewhere (but only when Huge Images?)


 .mrg.

From: Rin Okuyama <rokuyama.rk@gmail.com>
To: Lloyd Parkes <lloyd@must-have-coffee.gen.nz>,
 matthew green <mrg@NetBSD.org>, gnats-bugs@NetBSD.org
Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: toolchain/58663: gpt(1) biosboot.c doesn't compile on modern
 Linux
Date: Wed, 4 Sep 2024 14:23:45 +0900

 On 2024/09/04 13:33, matthew green wrote:
 >> -       daddr_t start = 0;
 >> +       off_t start = 0;        /* off_t because of gpt_human_get() */
 > 
 > i think your change is generally correct, but it really indicates
 > it much larger problem.
 > 
 > daddr_t should be made to be 64-bit. (except for the cases where netbsd
 > is building 32-bit daddr_t values for eg, old platform boot programs.)
 >   
 > ie, this seems like it is *also* a compat issue we should fix, it may
 > be causing weird issues elsewhere (but only when Huge Images?)

 Yeah. For Linux/glibc, it seems that daddr_t can be overridden in
 a similar manner to ours:

 https://github.com/bminor/glibc/blob/master/posix/sys/types.h#L113-L117
 (/usr/include/x86_64-linux-gnu/sys/types.h for Ubuntu/x86_64)

 Then, adding

 ````
 #ifdef __linux__
 #define	__daddr_t long long
 #endif
 ````

 to tools/compat/compat_defs, works around the problems for *most* cases.
 However,

 - Host library routines or syscalls may accept daddr_t arguments (and/or
    struct members). daddr_t is not portable, and we should avoid direct
    call for such routines although...
 - Other libc implementation may handles daddr_t differently.

 Thoughts?

 Lloyd, please modify `start` argument for biosboot() also. Otherwise,
 it gets truncated similarly.

 Thanks,
 rin

 P.S. Lloyd, welcome to our project :)

From: Jason Thorpe <thorpej@me.com>
To: gnats-bugs@netbsd.org
Cc: toolchain-manager@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 Lloyd Parkes <lloyd@must-have-coffee.gen.nz>
Subject: Re: toolchain/58663: gpt(1) biosboot.c doesn't compile on modern
 Linux
Date: Wed, 4 Sep 2024 06:31:47 -0700

 > On Sep 3, 2024, at 9:35=E2=80=AFPM, matthew green via gnats =
 <gnats-admin@NetBSD.org> wrote:
 >=20
 > The following reply was made to PR toolchain/58663; it has been noted =
 by GNATS.
 >=20
 > From: matthew green <mrg@eterna23.net>
 > To: gnats-bugs@netbsd.org
 > Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
 >    netbsd-bugs@netbsd.org
 > Subject: re: toolchain/58663: gpt(1) biosboot.c doesn't compile on =
 modern Linux
 > Date: Wed, 04 Sep 2024 14:33:04 +1000
 >=20
 >> -       daddr_t start =3D 0;
 >> +       off_t start =3D 0;        /* off_t because of gpt_human_get() =
 */
 >=20
 > i think your change is generally correct, but it really indicates
 > it much larger problem.
 >=20
 > daddr_t should be made to be 64-bit. (except for the cases where =
 netbsd
 > is building 32-bit daddr_t values for eg, old platform boot programs.)

 Be careful .. it certainly used to be the case that daddr_t was used in =
 on-disk structure definitions in e.g. UFS.

 -- thorpej

From: Jason Thorpe <thorpej@me.com>
To: gnats-bugs@netbsd.org
Cc: toolchain-manager@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 Lloyd Parkes <lloyd@must-have-coffee.gen.nz>
Subject: Re: toolchain/58663: gpt(1) biosboot.c doesn't compile on modern
 Linux
Date: Wed, 4 Sep 2024 06:31:47 -0700

 > On Sep 3, 2024, at 9:35=E2=80=AFPM, matthew green via gnats =
 <gnats-admin@NetBSD.org> wrote:
 >=20
 > The following reply was made to PR toolchain/58663; it has been noted =
 by GNATS.
 >=20
 > From: matthew green <mrg@eterna23.net>
 > To: gnats-bugs@netbsd.org
 > Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
 >    netbsd-bugs@netbsd.org
 > Subject: re: toolchain/58663: gpt(1) biosboot.c doesn't compile on =
 modern Linux
 > Date: Wed, 04 Sep 2024 14:33:04 +1000
 >=20
 >> -       daddr_t start =3D 0;
 >> +       off_t start =3D 0;        /* off_t because of gpt_human_get() =
 */
 >=20
 > i think your change is generally correct, but it really indicates
 > it much larger problem.
 >=20
 > daddr_t should be made to be 64-bit. (except for the cases where =
 netbsd
 > is building 32-bit daddr_t values for eg, old platform boot programs.)

 Be careful .. it certainly used to be the case that daddr_t was used in =
 on-disk structure definitions in e.g. UFS.

 -- thorpej

Responsible-Changed-From-To: toolchain-manager->lloyd
Responsible-Changed-By: lloyd@NetBSD.org
Responsible-Changed-When: Mon, 16 Sep 2024 00:03:01 +0000
Responsible-Changed-Why:
Assign the PR to myself.


From: Rin Okuyama <rokuyama.rk@gmail.com>
To: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org, lloyd@NetBSD.org
Cc: 
Subject: Re: toolchain/58663 gpt(1) biosboot.c doesn't compile on modern Linux
Date: Sat, 21 Sep 2024 13:04:20 +0900

 Note that gpt(8) itself should be fixed by this commit.

 Thanks,
 rin
 -------- Forwarded Message --------
 Subject: CVS commit: src/sbin/gpt
 Date: Fri, 13 Sep 2024 11:11:30 +0000
 From: Michael van Elst <mlelstv@netbsd.org>
 Reply-To: source-changes-d@NetBSD.org
 To: source-changes-full@NetBSD.org

 Module Name:	src
 Committed By:	mlelstv
 Date:		Fri Sep 13 11:11:30 UTC 2024

 Modified Files:
 	src/sbin/gpt: biosboot.c

 Log Message:
 Don't use kernel type daddr_t for disk offsets.


 To generate a diff of this commit:
 cvs rdiff -u -r1.32 -r1.33 src/sbin/gpt/biosboot.c

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


From: Lloyd Parkes <lloyd@must-have-coffee.gen.nz>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: toolchain/58663 gpt(1) biosboot.c doesn't compile on modern
 Linux
Date: Sun, 22 Sep 2024 10:20:09 +1200

 On Sat, 2024-09-21 at 04:05 +0000, Rin Okuyama via gnats wrote:
 > 
 >  Note that gpt(8) itself should be fixed by this commit.
 >  

 OK. I admit I have been a bit distracted recently. 

 I'll double check that the patch applies cleanly to netbsd-9 and
 netbsd-10 and then request pullups.

 Ngā mihi,
 Lloyd

State-Changed-From-To: open->pending-pullups
State-Changed-By: lloyd@NetBSD.org
State-Changed-When: Mon, 23 Sep 2024 00:18:22 +0000
State-Changed-Why:
[pullup-10 #911] Fix for gpt(8) compiled with GCC 14 on a Linux host


State-Changed-From-To: pending-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 09 Oct 2024 15:05:32 +0000
State-Changed-Why:
fixed in HEAD, pulled up to 10 (should this be pulled up to 9 too?)
pullup-10 #911 https://releng.netbsd.org/cgi-bin/req-10.cgi?show=911


From: Lloyd Parkes <lloyd@must-have-coffee.gen.nz>
To: gnats-bugs@netbsd.org, lloyd@netbsd.org, netbsd-bugs@netbsd.org, 
	gnats-admin@netbsd.org, riastradh@NetBSD.org
Cc: 
Subject: Re: toolchain/58663 (gpt(1) biosboot.c doesn't compile on modern
 Linux)
Date: Thu, 10 Oct 2024 12:19:40 +1300

 On Wed, 2024-10-09 at 15:05 +0000, riastradh@NetBSD.org wrote:
 > State-Changed-Why:
 > fixed in HEAD, pulled up to 10 (should this be pulled up to 9 too?)

 The pullup-9 ticket is #1889 and it has been completed as far as I can
 tell. I'm still getting the hang of how to put the information I want
 into GNATS.

 Ngā mihi,
 Lloyd

>Unformatted:

Home	PR Database Search