Re: Fix incorrect checksum generation in sr_meta_init

[Leo Unglaub]

Hey,

i cannot comment on the correctness of the change, but i have it in my 
tree running for 5 days now without any problems. This is not an OK, 
just feedback.


Greetings
Leo

On 27.03.22 01:20, Crystal Kolipe wrote:

--- softraid.c.dist Sat Mar 26 19:40:51 2022
+++ softraid.c  Sat Mar 26 20:59:46 2022
@@ -567,8 +567,6 @@
sizeof(scm->scmi.scm_devname));
memcpy(&scm->scmi.scm_uuid, &sm->ssdi.ssd_uuid,
sizeof(scm->scmi.scm_uuid));
-   sr_checksum(sc, scm, &scm->scm_checksum,
-   sizeof(scm->scm_checksum));
  
  		if (min_chunk_sz == 0)

min_chunk_sz = scm->scmi.scm_size;
@@ -580,9 +578,12 @@
  
  	sm->ssdi.ssd_secsize = secsize;
  
-	/* Equalize chunk sizes. */

-   SLIST_FOREACH(chunk, cl, src_link)
+   /* Equalize chunk sizes and calculate chunk checksum. */
+   SLIST_FOREACH(chunk, cl, src_link) {
chunk->src_meta.scmi.scm_coerced_size = min_chunk_sz;
+   sr_checksum(sc, scm, &scm->scm_checksum,
+   sizeof(struct sr_meta_chunk_invariant));
+   }
  
  	sd->sd_vol.sv_chunk_minsz = min_chunk_sz;

sd->sd_vol.sv_chunk_maxsz = max_chunk_sz;

Re: Fix incorrect checksum generation in sr_meta_init

[Theo de Raadt]

kqueue uses a userland data structure.

The netbsd PR is saying that the userland data structure gets corrupted,
so that the event doesn't arrive.  Thus the process stops.

That PR says their kernel is not buggy, but the kqueue handling.

If they are updated on libuv, maybe they are seeing the same libuv bug?



It sounds like a EVFILT_PROC is losing track of a child or exit.  I
don't know if cmake w/libuv is using EVFILT_PROC.  But if the userland
data structure is busted, well you can expect it to not notice
completion...

claudio did some minor work in signals, but I would expect any fallout from
his changes to affect lots of stuff, not just 1 program.

kqueue isn't like poll or select.  It has way more monitoring ability,
and if you screw up the userland memory, really weird things happen.

Fix incorrect checksum generation in sr_meta_init

[Crystal Kolipe]

Summary:

In function sr_meta_init in softraid.c, there are two bugs which cause the
chunk checksum stored in scm_checksum to be incorrectly calculated.  This
affects all newly created softraid volumes.


Details:

The MD5 checksum in scm_checksum should be calculated over the first 72 bytes
of struct sr_meta_chunk, that data being the contents of
struct sr_meta_chunk_invariant.

However, the current code, (since the bug was introduced in revision 1.262),
does the following:

sr_checksum(sc, scm, &scm->scm_checksum,
sizeof(scm->scm_checksum));

sizeof(scm->scm_checksum) always evaluates to MD5_DIGEST_LENGTH, or 16 bytes.

As a result, the checksum is calculated from just the members scm_volid,
scm_chunk_id, and the first eight bytes of the device name.  This leaves the
two chunk size and uuid fields unprotected by checksum.

To make matters worse, there is a second bug, because the code to set
scm_coerced_size is not run until _after_ the checksum has been calculated.

As a result, simply fixing the last argument to sr_checksum isn't sufficient,
as the checksum would then be performed on incomplete data, since
scm_coerced_size remains unset at this point.


Fix:

The following patch moves the call to sr_checksum into the second loop, and
corrects it's arguments.  This modified code produces valid checksums for the
whole of sr_meta_chunk_invariant.

Patch applies to -current and also applies cleanly to 7.0-release.

untrusted comment: verify with Exotic Silicon public signify key
RWRn5d3Yx35u0/6q18/WsBkdPkMziSWosd4n3NmI09dppZcyf653fXw7HzFxy+uACtRi1xC5uqg9w5bR/BtVWjonnA7t6diaoAQ=
--- softraid.c.dist Sat Mar 26 19:40:51 2022
+++ softraid.c  Sat Mar 26 20:59:46 2022
@@ -567,8 +567,6 @@
sizeof(scm->scmi.scm_devname));
memcpy(&scm->scmi.scm_uuid, &sm->ssdi.ssd_uuid,
sizeof(scm->scmi.scm_uuid));
-   sr_checksum(sc, scm, &scm->scm_checksum,
-   sizeof(scm->scm_checksum));
 
if (min_chunk_sz == 0)
min_chunk_sz = scm->scmi.scm_size;
@@ -580,9 +578,12 @@
 
sm->ssdi.ssd_secsize = secsize;
 
-   /* Equalize chunk sizes. */
-   SLIST_FOREACH(chunk, cl, src_link)
+   /* Equalize chunk sizes and calculate chunk checksum. */
+   SLIST_FOREACH(chunk, cl, src_link) {
chunk->src_meta.scmi.scm_coerced_size = min_chunk_sz;
+   sr_checksum(sc, scm, &scm->scm_checksum,
+   sizeof(struct sr_meta_chunk_invariant));
+   }
 
sd->sd_vol.sv_chunk_minsz = min_chunk_sz;
sd->sd_vol.sv_chunk_maxsz = max_chunk_sz;