Bug 233180

Summary:

Several errors in pmbr: 64-bits arithmetics and some others

Product:

Base System

Reporter:

Emrion <kmachine>

Component:

misc

Assignee:

Warner Losh <imp>

Status:

Closed FIXED

Severity:

Affects Some People

CC:

emaste, imp, kmachine

Priority:

---

Version:

11.2-RELEASE

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
pmbr source code patched	none

Description Emrion 2018-11-12 19:45:13 UTC

These issues have low impact because they require precise circumstances to trigger one of them. The disk must be > 2 TiB in size and either:
- The primary GPT header is dammaged.
- The freebsd-boot partiton is located farther than the first 2 TiB of the disc and one of its sectors takes place at a lba value that makes the higher 32 bits of this very value change.

Errors and corrections folow:


* Lines 117 - 118

main.3a:	decl (%si)			# 0x0(%si) = last sec (0-31)
		movw $2,%cx

Should be:
main.3a:	subl $1, (%si)			# 0x0(%si) = last sec (0-31)
		sbbl $0, 4(%si)
		movw $4,%cx

-> Copies only two 16-bits words but it's a 64-bits value. Moreover, decrements this 64-bit value without care for a possible carry.


* Line 131

movb $0x10,%cl
repe cmpsb

Should be:
movw $0x10,%cx
repe cmpsb

-> It's CX the counter for repe not CL. It works as is but it's dangerous to keep that.


* Lines 153 - 154

next_boot:	incl (%si)			# Next LBA
		adcl $0,4(%si)

Should be:
next_boot:	addl $1, (%si)			# Next LBA
		adcl $0,4(%si)

-> inc instruction doesn't affect the carry flag.


* Lines 174 - 175

incl GPT_ADDR+GPT_PART_LBA	# Next sector
adcl $0,GPT_ADDR+GPT_PART_LBA+4

Should be:
addl $1, GPT_ADDR+GPT_PART_LBA.
adcl $0,GPT_ADDR+GPT_PART_LBA+4

-> Same as before.

Comment 1 Emrion 2019-01-10 18:49:46 UTC

Created attachment 200996 [details]
pmbr source code patched

Comment 2 Emrion 2019-01-10 18:52:13 UTC

For your information, I compiled the code corrected as indicated above and installed it on a 12.0-RELEASE. It works.

However, I can't test the cases where the original pmbr would crash as I don't have a disk > 2 TiB.

Comment 3 Warner Losh freebsd_committer

2021-07-07 21:37:56 UTC

https://reviews.freebsd.org/D31100

Comment 4 Emrion 2021-07-08 19:48:01 UTC

Thanks for patching pmbr. There is also something more on the same idea concerning gptboot and zfsgptboot (not a bug but additional features). You can find it here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=235206

Comment 5 commit-hook freebsd_committer

2021-07-13 21:42:13 UTC

A commit in branch main references this bug:

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

commit 0ca9f1d4a3b772036309fb1c14262ec77c674c5d
Author:     Emrion <kmachine@free.fr>
AuthorDate: 2021-07-13 20:37:59 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-13 21:40:44 +0000

    Fix pmbr issues > 2TB

    These issues have low impact because they require precise circumstances
    to trigger one of them. The disk must be > 2 TiB in size and either:
    - The primary GPT header is dammaged.
    - The freebsd-boot partiton is located farther than the first 2 TiB of
      the disc and one of its sectors takes place at a lba value that makes
      the higher 32 bits of this very value change.
    Errors and corrections folow:
    - decl and incl don't affect CF, so replace with subl/addl $1
    - repe uses %cx, so move size to it with movw
    - moving a 64-bit value with %cx of 2 (should be 4) so addresses
      > 2TB will work.

    PR:                     233180
    Reviewed by:            imp@ (applied patch using description in bug)
    Differential Revision:  https://reviews.freebsd.org/D31100

 stand/i386/pmbr/pmbr.S | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comment 6 Warner Losh freebsd_committer

2021-07-13 21:43:47 UTC

Test booted in qemu. It works the same as before for small drives.
code looks good to my queue.
Pushed to main branch
queued to my MFC branches.

Comment 7 commit-hook freebsd_committer

2021-07-21 16:14:29 UTC

A commit in branch stable/13 references this bug:

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

commit b0b483b64be8533c8113500850ae8de2d127a98d
Author:     Emrion <kmachine@free.fr>
AuthorDate: 2021-07-13 20:37:59 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-21 16:13:10 +0000

    Fix pmbr issues > 2TB

    These issues have low impact because they require precise circumstances
    to trigger one of them. The disk must be > 2 TiB in size and either:
    - The primary GPT header is dammaged.
    - The freebsd-boot partiton is located farther than the first 2 TiB of
      the disc and one of its sectors takes place at a lba value that makes
      the higher 32 bits of this very value change.
    Errors and corrections folow:
    - decl and incl don't affect CF, so replace with subl/addl $1
    - repe uses %cx, so move size to it with movw
    - moving a 64-bit value with %cx of 2 (should be 4) so addresses
      > 2TB will work.

    PR:                     233180
    Reviewed by:            imp@ (applied patch using description in bug)
    Differential Revision:  https://reviews.freebsd.org/D31100

    (cherry picked from commit 0ca9f1d4a3b772036309fb1c14262ec77c674c5d)

 stand/i386/pmbr/pmbr.S | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comment 8 commit-hook freebsd_committer

2021-07-21 16:19:35 UTC

A commit in branch stable/12 references this bug:

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

commit 3cd395b3b9d2ab70d589d27203904238f138442f
Author:     Emrion <kmachine@free.fr>
AuthorDate: 2021-07-13 20:37:59 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-21 16:16:31 +0000

    Fix pmbr issues > 2TB

    These issues have low impact because they require precise circumstances
    to trigger one of them. The disk must be > 2 TiB in size and either:
    - The primary GPT header is dammaged.
    - The freebsd-boot partiton is located farther than the first 2 TiB of
      the disc and one of its sectors takes place at a lba value that makes
      the higher 32 bits of this very value change.
    Errors and corrections folow:
    - decl and incl don't affect CF, so replace with subl/addl $1
    - repe uses %cx, so move size to it with movw
    - moving a 64-bit value with %cx of 2 (should be 4) so addresses
      > 2TB will work.

    PR:                     233180
    Reviewed by:            imp@ (applied patch using description in bug)
    Differential Revision:  https://reviews.freebsd.org/D31100

    (cherry picked from commit 0ca9f1d4a3b772036309fb1c14262ec77c674c5d)

 stand/i386/pmbr/pmbr.s | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)