Merge ~mfo/casper:lp1892369 into casper:main

Proposed by Mauricio Faria de Oliveira
Status: Merged
Merged at revision: 0208d5f99950947bbbb3483b5c311590c59dc106
Proposed branch: ~mfo/casper:lp1892369
Merge into: casper:main
Diff against target: 45 lines (+14/-3)
2 files modified
casper-md5check/casper-md5check.c (+7/-3)
debian/changelog (+7/-0)
Reviewer Review Type Date Requested Status
Ubuntu Installer Team Pending
Review via email: mp+417120@code.staging.launchpad.net

Description of the change

casper-md5check fails to detect 'fsck.mode=skip' if it's the last option in /proc/cmdline; fix that.

Tests
=====

Before:
---

$ sudo apt install --yes casper
$ dpkg -s casper | grep Version:
Version: 1.467

1) Without fsck.mode=skip: check occurs (OK)

$ cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.13.0-28-generic root=/dev/mapper/nvme--vg-root ro quiet splash mitigations=off vt.handoff=7

$ sudo /usr/lib/casper/casper-md5check /tmp /dev/zero
.
Checking integrity, this may take some time
^C

2) With fsck.mode=skip last: check occurs (WRONG!)

$ echo "$(cat /proc/cmdline) fsck.mode=skip" >/tmp/cmdline
$ sudo mount --bind /tmp/cmdline /proc/cmdline
$ cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.13.0-28-generic root=/dev/mapper/nvme--vg-root ro quiet splash mitigations=off vt.handoff=7 fsck.mode=skip

$ sudo /usr/lib/casper/casper-md5check /tmp /dev/zero
.
Checking integrity, this may take some time
^C

$ sudo umount /proc/cmdline

3) With fsck.mode=skip before last: check skipped (OK; workaround available)

$ echo "$(cat /proc/cmdline) fsck.mode=skip workaround" >/tmp/cmdline
$ sudo mount --bind /tmp/cmdline /proc/cmdline
$ cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.13.0-28-generic root=/dev/mapper/nvme--vg-root ro quiet splash mitigations=off vt.handoff=7 fsck.mode=skip workaround

ubuntu@jammy:~$ sudo /usr/lib/casper/casper-md5check /tmp /dev/zero
.
Check skipped.

$ sudo umount /proc/cmdline

After:
---

$ sudo dpkg -i casper_1.468_amd64.deb

1) Without fsck.mode=skip: check occurs (OK), and hint is provided

$ sudo umount /proc/cmdline
$ cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.13.0-28-generic root=/dev/mapper/nvme--vg-root ro quiet splash mitigations=off vt.handoff=7

$ sudo /usr/lib/casper/casper-md5check /tmp /dev/zero
.
Checking integrity, this may take some time (or try: fsck.mode=skip)
^C

2) With fsck.mode=skip last: check skipped (OK; FIXED!)

$ echo "$(cat /proc/cmdline) fsck.mode=skip" >/tmp/cmdline
$ sudo mount --bind /tmp/cmdline /proc/cmdline
$ cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.13.0-28-generic root=/dev/mapper/nvme--vg-root ro quiet splash mitigations=off vt.handoff=7 fsck.mode=skip

$ sudo /usr/lib/casper/casper-md5check /tmp /dev/zero
.
Check skipped.

$ sudo umount /proc/cmdline

3) With fsck.mode=skip before last: check skipped (OK; no regression)

$ echo "$(cat /proc/cmdline) fsck.mode=skip workaround" >/tmp/cmdline
$ sudo mount --bind /tmp/cmdline /proc/cmdline
$ cat /proc/cmdline
BOOT_IMAGE=/vmlinuz-5.13.0-28-generic root=/dev/mapper/nvme--vg-root ro quiet splash mitigations=off vt.handoff=7 fsck.mode=skip workaround

ubuntu@jammy:~$ sudo /usr/lib/casper/casper-md5check /tmp /dev/zero
.
Check skipped.

Details:
=======

Note it ends with '\n'.

 $ cat /proc/cmdline
 BOOT_IMAGE=/vmlinuz-<...> vt.handoff=7

 $ hexdump -C /proc/cmdline
 00000000 42 4f 4f 54 5f 49 4d 41 47 45 3d 2f 76 6d 6c 69 |BOOT_IMAGE=/vmli|
 <...>
 00000060 3d 6f 66 66 20 76 74 2e 68 61 6e 64 6f 66 66 3d |=off vt.handoff=|
 00000070 37 0a |7.|
 00000072

Test-case from original source,
added 3 printf()s for debugging.

@ cmdline.c

 #define _GNU_SOURCE
 #include <stdio.h>
 #include <string.h>

 void parse_cmdline(void) {
   FILE *cmdline = fopen("/proc/cmdline", "r");
   char buf[1024];
   char *bufp = buf, *tok;
   char *theme;

   if (!cmdline)
     return;

   fread(buf, 1023, 1, cmdline);
   buf[strlen(buf)] = '\0';
   printf("strlen(buf) %ld\n", strlen(buf));

   while ((tok = strsep(&bufp, " ")) != NULL) {
     printf("tok %p, bufp %p, string '%s'\n", tok, bufp, tok);
     if (strncmp(tok, "vt.handoff=7", sizeof("vt.handoff=7")) == 0)
       printf("Match found\n");
   }

   fclose(cmdline);

 }

 int main(void) {
  parse_cmdline();
  return 0;
 }

$ gcc -o cmdline cmdline.c

Issue #1: strlen() gets wrong length
---

$ ./cmdline
strlen(buf) 118
tok 0x7fffa3864b60, bufp 0x7fffa3864b86, string 'BOOT_IMAGE=/vmlinuz-5.13.0-28-generic'
tok 0x7fffa3864b86, bufp 0x7fffa3864ba5, string 'root=/dev/mapper/nvme--vg-root'
tok 0x7fffa3864ba5, bufp 0x7fffa3864ba8, string 'ro'
tok 0x7fffa3864ba8, bufp 0x7fffa3864bae, string 'quiet'
tok 0x7fffa3864bae, bufp 0x7fffa3864bb5, string 'splash'
tok 0x7fffa3864bb5, bufp 0x7fffa3864bc5, string 'mitigations=off'
tok 0x7fffa3864bc5, bufp (nil), string 'vt.handoff=7
>S�'

Issue #1: 0x72 == 114, strlen(buf) == 118, so '\0' allowed 3 extra chars in the buf (junk)

Fix #1: use memset instead.

$ ./cmdline
strlen(buf) 114
tok 0x7fffa7ed5900, bufp 0x7fffa7ed5926, string 'BOOT_IMAGE=/vmlinuz-5.13.0-28-generic'
tok 0x7fffa7ed5926, bufp 0x7fffa7ed5945, string 'root=/dev/mapper/nvme--vg-root'
tok 0x7fffa7ed5945, bufp 0x7fffa7ed5948, string 'ro'
tok 0x7fffa7ed5948, bufp 0x7fffa7ed594e, string 'quiet'
tok 0x7fffa7ed594e, bufp 0x7fffa7ed5955, string 'splash'
tok 0x7fffa7ed5955, bufp 0x7fffa7ed5965, string 'mitigations=off'
tok 0x7fffa7ed5965, bufp (nil), string 'vt.handoff=7
'

See strlen(buf) is 114 now.

Issue #2: newline in last parameter
---

Notice a newline in last line above, and no match found.

Since strsep() didn't find a delimiter after the last token,
it doesn't overwrite that delimiter with '\0' thus strncmp()
doesn't match because sizeof("string") counts that '\0' char.

Fix #2: add '\n' as another delimiter byte in the string

$ ./cmdline
tok 0x7ffeed9d04b0, bufp 0x7ffeed9d04d6, string 'BOOT_IMAGE=/vmlinuz-5.13.0-28-generic'
tok 0x7ffeed9d04d6, bufp 0x7ffeed9d04f5, string 'root=/dev/mapper/nvme--vg-root'
tok 0x7ffeed9d04f5, bufp 0x7ffeed9d04f8, string 'ro'
tok 0x7ffeed9d04f8, bufp 0x7ffeed9d04fe, string 'quiet'
tok 0x7ffeed9d04fe, bufp 0x7ffeed9d0505, string 'splash'
tok 0x7ffeed9d0505, bufp 0x7ffeed9d0515, string 'mitigations=off'
tok 0x7ffeed9d0515, bufp 0x7ffeed9d0522, string 'vt.handoff=7'
Match found
tok 0x7ffeed9d0522, bufp (nil), string ''

Issue #3: another loop iteration after the last token
---

It happens because we don't check for the token not found case
in strsep() (see manpage) which sets bufp to NULL.

In our case shouldnt be harmful, but for correctness, let's fix it.

Fix #3: '&& bufp != NULL' to the conditional

$ ./cmdline
tok 0x7ffce3426750, bufp 0x7ffce3426776, string 'BOOT_IMAGE=/vmlinuz-5.13.0-28-generic'
tok 0x7ffce3426776, bufp 0x7ffce3426795, string 'root=/dev/mapper/nvme--vg-root'
tok 0x7ffce3426795, bufp 0x7ffce3426798, string 'ro'
tok 0x7ffce3426798, bufp 0x7ffce342679e, string 'quiet'
tok 0x7ffce342679e, bufp 0x7ffce34267a5, string 'splash'
tok 0x7ffce34267a5, bufp 0x7ffce34267b5, string 'mitigations=off'
tok 0x7ffce34267b5, bufp 0x7ffce34267c2, string 'vt.handoff=7'
Match found

To post a comment you must log in.
Revision history for this message
Brian Murray (brian-murray) wrote :

I've merged and uploaded this, thanks for the detailed testing!

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches