- Download Diff

Summary:

6005 bootadm update-archive should create/update archive sha-1 hash

Review Request #56 — Created June 6, 2015 and submitted Aug. 4, 2016, 5:09 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	6005
Depends On:
Commit:	6646d59...
Reviewers
Groups:	general
People:

Description

Testing Done

root@openindiana:/tmp# ./bootadm update-archive -f
updating //platform/i86pc/boot_archive
updating //platform/i86pc/amd64/boot_archive
root@openindiana:/tmp# more //platform/i86pc/boot_archive.hash
c5cc0f9801319b8a699656b5e3fbb0ee2e27ea47
root@openindiana:/tmp# more //platform/i86pc/amd64/boot_archive.hash
29c171b6a97422e4e0f2b3f47115d8117c80711e
root@openindiana:/tmp# digest -v -a sha1 /platform/i86pc/boot_archive
sha1 (/platform/i86pc/boot_archive) = c5cc0f9801319b8a699656b5e3fbb0ee2e27ea47
root@openindiana:/tmp# digest -v -a sha1 /platform/i86pc/amd64/boot_archive
sha1 (/platform/i86pc/amd64/boot_archive) = 29c171b6a97422e4e0f2b3f47115d8117c80711e

Issues

0
0
8
0
All Issues: 8

Description	From	Last Updated
Use sizeof(archive_hash) instead of PATH_MAX.	yuripv	March 25, 2016, 10:21 a.m.
But you ARE in create_ramdisk(), do you mean the ksh script with the same name?	yuripv	March 25, 2016, 10:20 a.m.
The 'digest' name could easily create name collisions, beadm_digest() may be?	yuripv	March 25, 2016, 10:20 a.m.
The "please check" part sounds somewhat excessive :-)	yuripv	March 25, 2016, 10:20 a.m.
Why NULL_PTR? You are using standard malloc() after all.	yuripv	March 25, 2016, 10:19 a.m.
Simple "out of memory" without assigning err=errno and using strerror() would be enough here, you aren't handling the EAGAIN anyway. ...	yuripv	March 25, 2016, 10:18 a.m.
Here and below - don't need the NULL checks, userland free() does that for you.	yuripv	March 25, 2016, 10:18 a.m.
Why is this checking down here, rather than right after the read loop?	richlowe	July 25, 2016, 1:07 p.m.

Change Summary:

bam_print() calls need newline terminated strings.

Commit:

-	ad7a688124c922a68df05a9f38e292ac777eaecd
+	bf4fe6b353489416797b56260ba6c4aab25599f5

Diff:

Revision 2 (+286 -2)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

It would be nice if there was some way to verify the boot_archive on boot using the hash file. Is that a planned feature?

tsoome June 6, 2015, 11:42 a.m.

yes, see:
https://www.illumos.org/issues/5886
https://www.illumos.org/issues/5887
https://us-east.manta.joyent.com/rmustacc/public/webrevs/5886/index.html (see dboot_startkern.c)

the bootfs update also has update to verify the module hashes. As the 5886/5887 are in process to be reviewd, I did address this request to be request for comments in dev list - once bootfs will progress and people think this change is way to go, I'll file an RFE.

rm June 6, 2015, 2:50 p.m.

Actually, verifying the hash is already present for the boot archive. That was integrated in https://www.illumos.org/issues/3364.

andy_js June 15, 2015, 3:02 p.m.

How would I go about using that functionality?

tsoome June 15, 2015, 3:23 p.m.

with current kernel and grub, it should be enough to add following line to specific menu entry, after boot_archive itself:
module$ /platform/i86pc/$ISADIR/boot_archive.hash

however, to actually see if the hash was checked, you need to add -B prom_debug=1 to kernel options, also it will help if you can direct console to serial port and capture - prom_debug will spit out a lot of data.

a bit indirect way us to edit hash file and change hash value - if the hash is checked and it fails, you will get panic. with this strategy, the panic with bad hash will tell you that hash check indeed was performed.

Change Summary:

include issue number.

Description:

~		bootadm archive-update should create archive hash as well
	~	6005 bootadm archive-update should create archive hash

Bugs:

+	6005

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Change Summary:

as preparing for RTI: update to make git-pbchk happy. whitespace issue in bootadm_digest.c and updated copyright messages.

Commit:

-	bf4fe6b353489416797b56260ba6c4aab25599f5
+	d1dd0226845dfd534613afbf24f5ab4b3e3cdd80

Diff:

Revision 3 (+291 -2)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Commit:

-	d1dd0226845dfd534613afbf24f5ab4b3e3cdd80
+	974a109076bf38c8bd4670f1e2f95cd315d56579

Diff:

Revision 4 (+291 -2)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Change Summary:

updated copyright to be consistent with digest.c (as the main source of this work).

Commit:

-	974a109076bf38c8bd4670f1e2f95cd315d56579
+	6646d59025e58ddf97129c8e522348fcead226fb

Diff:

Revision 5 (+304 -2)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 5)

Show all issues

Use sizeof(archive_hash) instead of PATH_MAX.

tsoome March 25, 2016, 10:21 a.m.

reworked to use asprintf() instead. if path is getting too long, fopen() will fail and error will be set.

usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 5)

Show all issues

But you ARE in create_ramdisk(), do you mean the ksh script with the same name?

usr/src/cmd/boot/bootadm/bootadm_digest.c (Diff revision 5)

Show all issues

The 'digest' name could easily create name collisions, beadm_digest() may be?

usr/src/cmd/boot/bootadm/bootadm_digest.c (Diff revision 5)

Show all issues

The "please check" part sounds somewhat excessive :-)

tsoome March 25, 2016, 10:21 a.m.

removed it; was carried over from digest source, but bootadm is run by admin, so this message is not needed.

usr/src/cmd/boot/bootadm/bootadm_digest.c (Diff revision 5)
Show all issues
```
Why NULL_PTR? You are using standard malloc() after all.
```
1. tsoome March 25, 2016, 10:21 a.m.
  yes, carried over from digest command...

usr/src/cmd/boot/bootadm/bootadm_digest.c (Diff revision 5)

Show all issues

Simple "out of memory" without assigning err=errno and using strerror() would be enough here, you aren't handling the EAGAIN anyway.

Same for lines 202 and 231.

usr/src/cmd/boot/bootadm/bootadm_digest.c (Diff revision 5)

Show all issues

Here and below - don't need the NULL checks, userland free() does that for you.

Change Summary:

changes suggested by Yuri

Summary:

-	bootadm archive-update should create archive hash as well
+	6005 bootadm update-archive should create/update archive sha-1 hash

Description:

~		6005 bootadm archive-update should create archive hash
	~	6005 bootadm update-archive should create/update archive sha-1 hash

Diff:

Revision 6 (+305 -9)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Ship it!

```
Ship It!
```

Change Summary:

rebase on 6314 buffer overflow in dsl_dataset_name

Diff:

Revision 7 (+303 -2)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

usr/src/cmd/boot/bootadm/bootadm_digest.c (Diff revision 7)

Show all issues

Why is this checking down here, rather than right after the read loop?

tsoome July 25, 2016, 1:07 p.m.

Right, this is oversighted leftover from digest. Since digest is processing multiple files, that code had to complete disgest update step. Since bootadm does not digest multiple files in same PKI session, we can return at once and close the session. Thanks for spotting this issue:)

Change Summary:

Fix based on comment from Rich.

Diff:

Revision 8 (+295 -2)

Show changes

	usr/src/cmd/boot/bootadm/Makefile
	usr/src/cmd/boot/bootadm/bootadm.h
	usr/src/cmd/boot/bootadm/bootadm.c
	usr/src/cmd/boot/bootadm/bootadm_digest.c
	usr/src/cmd/boot/scripts/create_ramdisk.ksh

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

6005 bootadm update-archive should create/update archive sha-1 hash

Change Summary:

Commit:

Diff:

Change Summary:

Description:

6005 bootadm archive-update should create archive hash

Bugs:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Summary:

Description:

6005 bootadm archive-update should create archive hash

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)