NetBSD Problem Report #52522
From www@NetBSD.org Sun Sep 3 13:15:41 2017
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-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 AF3D87A0F8
for <gnats-bugs@gnats.NetBSD.org>; Sun, 3 Sep 2017 13:15:41 +0000 (UTC)
Message-Id: <20170903131540.412C47A275@mollari.NetBSD.org>
Date: Sun, 3 Sep 2017 13:15:40 +0000 (UTC)
From: aniou@smutek.pl
Reply-To: aniou@smutek.pl
To: gnats-bugs@NetBSD.org
Subject: incorrect partition label handling in gpt(8) and kernel (wedges)
X-Send-Pr-Version: www-1.0
>Number: 52522
>Category: kern
>Synopsis: incorrect partition label handling in gpt(8) and kernel (wedges)
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: jnemeth
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sun Sep 03 13:20:00 +0000 2017
>Closed-Date:
>Last-Modified: Mon Jun 04 22:04:42 +0000 2018
>Originator: Piotr Meyer
>Release: 8.0_BETA
>Organization:
>Environment:
NetBSD localhost 8.0_BETA NetBSD 8.0_BETA (GENERIC.201708300150Z) amd64
>Description:
gpt(8) and kernel incorrectly treats GPT partition labels as NULL-terminated strings[2] (in fact label is a "36 UTF-16LE code units", typically padded by zeros if they are shorter than 36 units, but NOT zero-terminated!)[1].
This behaviour leads to:
- inconsistencies between auto-generated wedge names and user-submitted ones
- truncating valid names (thus - there is impossible to use valid UUID as partition and wedge name when gpt(*) is used)
- incorrect naming of wedges from GPT partitions created on other systems (kernel expects null-terminated strings and do not check limits)
1 - see https://en.wikipedia.org/wiki/GUID_Partition_Table or compare with behaviour in other systems.
2 - https://nxr.netbsd.org/xref/src/sbin/gpt/gpt.c#227
>How-To-Repeat:
Create some partition by GPT:
# partition without label, wedge name created by kernel - valid UUID
localhost# gpt add -b 34 -s 33264 -t ffs wd1
/dev/rwd1: Partition 1 added: 49f48d5a-b10e-11dc-b99b-0019d1879648 34 33264
dmesg: dk0 at wd1: "58589a53-8481-4f5d-b415-c92e0aed99f4", 33264 blocks at 34, type: ffs
# short name, terminated by gpt(8) with zero, valid name and valid wedge name
localhost# gpt add -l testshort -b $((34+33264)) -s 33264 -t ffs wd1
/dev/rwd1: Partition 2 added: 49f48d5a-b10e-11dc-b99b-0019d1879648 33298 33264
dmesg: dk1 at wd1: "testshort", 33264 blocks at 33298, type: ffs
# [some wedges skipped]
# partition name with valid label - 36 bytes - as you can see label is truncated, as well as wedge name
localhost# gpt add -l 123456789012345678901234567890123456 -b $((99826+33264)) -s 33264 -t ffs wd1
/dev/rwd1: Partition 5 added: 49f48d5a-b10e-11dc-b99b-0019d1879648 133090 33264
dmesg: dk4 at wd1: "12345678901234567890123456789012345", 33264 blocks at 133090, type: ffs
# partition named by UUID create by uuidgen
localhost# uuidgen
b7462824-5cd3-42f8-9d7c-e0b2cd6da6da
# UUID is trunctated
localhost# gpt add -l b7462824-5cd3-42f8-9d7c-e0b2cd6da6da -b $((133090+33264)) -s 33264 -t ffs wd1
/dev/rwd1: Partition 6 added: 49f48d5a-b10e-11dc-b99b-0019d1879648 166354 33264
dmesg: dk5 at wd1: "b7462824-5cd3-42f8-9d7c-e0b2cd6da6d", 33264 blocks at 166354, type: ffs
#
#
# !!! example for wedge create automatically from partition created by parted on linux
#
#
[snip]
dk5 at wd1: "b7462824-5cd3-42f8-9d7c-e0b2cd6da6d", 33264 blocks at 166354, type: ffs
dk6 at wd1: "123456789012345678901234567890123456M-cM-6M-/M-`M-?M^FM-hM^RM^CM-dM^]M-2M-gM-&M^NM-fM-$M-=M-dM^_M^XM-nM^QM-=M-\M-7M-QM^MM-gM-5M-!M-dM^LM-3M-nM-.M^MM-kM-4M^VM-eM-#M-.M-cM^\M->M-oM^@M^@^C", 58195 blocks at 199618, type: <unknown>
dk7 at wd1: "123456789012345678901234567890123456", 30720 blocks at 258048, type: <unknown>
[snip]
# partition table
localhost# gpt show -l wd1
start size index contents
0 1 PMBR
1 1 Pri GPT header
2 32 Pri GPT table
34 33264 1 GPT part -
33298 33264 2 GPT part - testshort
66562 33264 3 GPT part - ccccccccccccccccccccccccccccccccccc
99826 33264 4 GPT part - ccccccccccccccccccccccccccccccccccc
133090 33264 5 GPT part - 12345678901234567890123456789012345
166354 33264 6 GPT part - b7462824-5cd3-42f8-9d7c-e0b2cd6da6d
199618 58195 7 GPT part - 123456789012345678901234567890123456㶯࿆蒃䝲禎椽䟘ܷэ絡䌳봖壮㜾
257813 235 Unused
258048 30720 8 GPT part - 123456789012345678901234567890123456
288768 3905503 Unused
4194271 32 Sec GPT table
4194303 1 Sec GPT header
localhost#
#
# wedges - look at inconsistency in autogenerated wedge names and truncated by gpt(8) ones
# - dk0 and dk3 vs dk4 or dk5
#
localhost# dkctl wd1 show
dkctl: unknown command: show
localhost# dkctl wd1 listwedges
/dev/rwd1: 8 wedges:
dk0: 58589a53-8481-4f5d-b415-c92e0aed99f4, 33264 blocks at 34, type: ffs
dk1: testshort, 33264 blocks at 33298, type: ffs
dk2: ccccccccccccccccccccccccccccccccccc, 33264 blocks at 66562, type: ffs
dk3: 0dcfa3a0-890e-42e4-866d-2d8b49f2ed24, 33264 blocks at 99826, type: ffs
dk4: 12345678901234567890123456789012345, 33264 blocks at 133090, type: ffs
dk5: b7462824-5cd3-42f8-9d7c-e0b2cd6da6d, 33264 blocks at 166354, type: ffs
dk6: 123456789012345678901234567890123456㶯࿆蒃䝲禎椽䟘ܷэ絡䌳봖壮㜾, 58195 blocks at 199618, type:
dk7: 123456789012345678901234567890123456, 30720 blocks at 258048, type:
>Fix:
1. Change gpt(8) behavior:
- do not assume that label is NULL-terminated - instead check for NULL or buffer end (72 bytes/376 UTF-16 chars)
- maybe pre-fill dst utf-18 buffer by NULLS to gracefully handle short names but do not replace last UTF-16 by NULL as they do now
2. Change kernel behaviour - do not assume that GPT label is null-terminated, see https://nxr.netbsd.org/xref/src/sys/dev/dkwedge/dkwedge_gpt.c#107
>Release-Note:
>Audit-Trail:
Responsible-Changed-From-To: kern-bug-people->jnemeth
Responsible-Changed-By: jnemeth@NetBSD.org
Responsible-Changed-When: Sun, 03 Sep 2017 20:16:39 +0000
Responsible-Changed-Why:
I'll take this as somebody that has done a lot of work on gpt(8), although
I'm not responsible for the broken code. Note that this should really be
two PRs: one for gpt(8) and one for the kernel.
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52522 CVS commit: src/sbin/gpt
Date: Tue, 5 Sep 2017 14:30:46 -0400
Module Name: src
Committed By: christos
Date: Tue Sep 5 18:30:46 UTC 2017
Modified Files:
src/sbin/gpt: gpt.c
Log Message:
PR/52522: Piotr Meyer: Don't NUL terminate the gpt label name.
XXX: pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.70 -r1.71 src/sbin/gpt/gpt.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Piotr Meyer <aniou@smutek.pl>
To: gnats-bugs@NetBSD.org
Cc: jnemeth@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/52522 CVS commit: src/sbin/gpt
Date: Wed, 6 Sep 2017 16:54:14 +0200
On Tue, Sep 05, 2017 at 06:35:01PM +0000, Christos Zoulas wrote:
>
> Modified Files:
> src/sbin/gpt: gpt.c
>
> Log Message:
> PR/52522: Piotr Meyer: Don't NUL terminate the gpt label name.
> XXX: pullup-8
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.70 -r1.71 src/sbin/gpt/gpt.c
I'm afraid that is too early to pullup - truncating long labels
by NUL is only small fraction of problem and without number of
other changes things are now worse: now, we are able to create
label with max-length (36 UTF-16 characters) but shorter labels,
created by NetBSD are not valid - there is no "end mark" (NUL)
(I assume that destination buffer isn't zeroed during allocation).
In the same time userland and kernel parts that assume
that string IS NUL-terminated are still here, for example in gpt:
https://nxr.netbsd.org/xref/src/sbin/gpt/gpt.c?r=1.71#132
(but there may be other places as well).
--
Piotr 'aniou' Meyer
From: christos@zoulas.com (Christos Zoulas)
To: Piotr Meyer <aniou@smutek.pl>, gnats-bugs@NetBSD.org
Cc: jnemeth@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/52522 CVS commit: src/sbin/gpt
Date: Wed, 6 Sep 2017 14:18:02 -0400
On Sep 6, 4:54pm, aniou@smutek.pl (Piotr Meyer) wrote:
-- Subject: Re: PR/52522 CVS commit: src/sbin/gpt
| On Tue, Sep 05, 2017 at 06:35:01PM +0000, Christos Zoulas wrote:
| >
| > Modified Files:
| > src/sbin/gpt: gpt.c
| >
| > Log Message:
| > PR/52522: Piotr Meyer: Don't NUL terminate the gpt label name.
| > XXX: pullup-8
| >
| >
| > To generate a diff of this commit:
| > cvs rdiff -u -r1.70 -r1.71 src/sbin/gpt/gpt.c
|
| I'm afraid that is too early to pullup - truncating long labels
| by NUL is only small fraction of problem and without number of
| other changes things are now worse: now, we are able to create
| label with max-length (36 UTF-16 characters) but shorter labels,
| created by NetBSD are not valid - there is no "end mark" (NUL)
| (I assume that destination buffer isn't zeroed during allocation).
|
| In the same time userland and kernel parts that assume
| that string IS NUL-terminated are still here, for example in gpt:
| https://nxr.netbsd.org/xref/src/sbin/gpt/gpt.c?r=1.71#132
| (but there may be other places as well).
You are right, fixed in userland.
christos
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52522 CVS commit: src/sys/dev/dkwedge
Date: Wed, 6 Sep 2017 14:21:17 -0400
Module Name: src
Committed By: christos
Date: Wed Sep 6 18:21:17 UTC 2017
Modified Files:
src/sys/dev/dkwedge: dkwedge_gpt.c
Log Message:
PR/52522: ent_name is not necessarily 0 terminated, so check bounds.
XXX: pullup-8
To generate a diff of this commit:
cvs rdiff -u -r1.18 -r1.19 src/sys/dev/dkwedge/dkwedge_gpt.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52522 CVS commit: [netbsd-8] src
Date: Mon, 12 Feb 2018 04:05:07 +0000
Module Name: src
Committed By: snj
Date: Mon Feb 12 04:05:07 UTC 2018
Modified Files:
src/sbin/gpt [netbsd-8]: backup.c biosboot.c gpt.c gpt.h restore.c
show.c
src/sys/dev/dkwedge [netbsd-8]: dkwedge_gpt.c
Log Message:
Pull up following revision(s) (requested by christos in ticket #545):
sbin/gpt/backup.c: 1.17-1.18
sbin/gpt/biosboot.c: 1.29-1.30
sbin/gpt/gpt.c: 1.71-1.73
sbin/gpt/gpt.h: 1.36
sbin/gpt/restore.c: 1.17
sbin/gpt/show.c: 1.40-1.41
sys/dev/dkwedge/dkwedge_gpt.c: 1.19-1.20
PR/52522: Piotr Meyer: Don't NUL terminate the gpt label name.
--
- make sure that the utf16 string is padded with 0's where needed.
- since the utf16 string is not 0 terminated, pass the size of the string.
--
use __arraycount
--
PR/52522: ent_name is not necessarily 0 terminated, so check bounds.
--
use arraycount.
To generate a diff of this commit:
cvs rdiff -u -r1.16 -r1.16.8.1 src/sbin/gpt/backup.c src/sbin/gpt/restore.c
cvs rdiff -u -r1.27.4.1 -r1.27.4.2 src/sbin/gpt/biosboot.c
cvs rdiff -u -r1.70 -r1.70.4.1 src/sbin/gpt/gpt.c
cvs rdiff -u -r1.35 -r1.35.4.1 src/sbin/gpt/gpt.h
cvs rdiff -u -r1.39 -r1.39.6.1 src/sbin/gpt/show.c
cvs rdiff -u -r1.18 -r1.18.6.1 src/sys/dev/dkwedge/dkwedge_gpt.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->feedback
State-Changed-By: maya@NetBSD.org
State-Changed-When: Mon, 04 Jun 2018 11:15:50 +0000
State-Changed-Why:
Hi. what's the status of this bug? it looks like it might have been fixed, are there remaining issues?
From: Piotr Meyer <aniou@smutek.pl>
To: gnats-bugs@NetBSD.org
Cc: jnemeth@NetBSD.org, netbsd-bugs@netbsd.org, gnats-admin@netbsd.org,
maya@NetBSD.org
Subject: Re: kern/52522 (incorrect partition label handling in gpt(8) and
kernel (wedges))
Date: Mon, 4 Jun 2018 16:05:45 +0200
On Mon, Jun 04, 2018 at 11:15:50AM +0000, maya@NetBSD.org wrote:
> Synopsis: incorrect partition label handling in gpt(8) and kernel (wedges)
>
> State-Changed-From-To: open->feedback
> State-Changed-By: maya@NetBSD.org
> State-Changed-When: Mon, 04 Jun 2018 11:15:50 +0000
> State-Changed-Why:
> Hi. what's the status of this bug? it looks like it might have been fixed, are there remaining issues?
The bug itself may be closed now, although it was only a part of larger
problem - a broken GPT partition management in sysinst. There[1] are, at
least, two problems here (if I rememeber correctly):
- [probably] invalid calls for dkctl or gpt commands (bad parameters/bad
parameters order - tools were changed in meanwhile)
- inflexible and dangerous algorithm that involves removing other
partitions and renaming newly created ones[2]:
Both of them leads to errors like [3] and makes sysinst dangerous when
NetBSD is installed on GPT-partitioned disk alongside of other OS-es.
I have had an idea about using UUID's as universally unique identifiers
for partitions and wedges, but then kern/52522 appeared.
1 - https://nxr.netbsd.org/xref/src/usr.sbin/sysinst/partman.c#1760
2 - https://nxr.netbsd.org/xref/src/usr.sbin/sysinst/partman.c#1775
3 - https://mail-index.netbsd.org/current-users/2014/10/26/msg026021.html
Best regards,
--
Piotr 'aniou' Meyer
From: Martin Husemann <martin@duskware.de>
To: Piotr Meyer <aniou@smutek.pl>
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/52522 (incorrect partition label handling in gpt(8) and
kernel (wedges))
Date: Mon, 4 Jun 2018 17:27:48 +0200
On Mon, Jun 04, 2018 at 04:05:45PM +0200, Piotr Meyer wrote:
> Both of them leads to errors like [3] and makes sysinst dangerous when
> NetBSD is installed on GPT-partitioned disk alongside of other OS-es.
Indeed this is still the case in -current.
Martin
State-Changed-From-To: feedback->closed
State-Changed-By: maya@NetBSD.org
State-Changed-When: Mon, 04 Jun 2018 21:44:12 +0000
State-Changed-Why:
The original bug is fixed. sysinst not handling gpt well is still unfixed. fwiw, there is some work by martin on sysinst to handle it better.
State-Changed-From-To: closed->open
State-Changed-By: jnemeth@NetBSD.org
State-Changed-When: Mon, 04 Jun 2018 22:04:42 +0000
State-Changed-Why:
This likely needs a number of pullups.
Don't close PRs for which I'm responsible!
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home