From: Miroslav Cimerman <mc@doas.su>
Subject: Re: softraid: incorrect chunk metadata checksum
To: Crystal Kolipe <kolipe.c@exoticsilicon.com>, "tech@openbsd.org" <tech@openbsd.org>
Date: Wed, 23 Apr 2025 08:44:23 +0000

Hi,

Crystal Kolipe <kolipe.c@exoticsilicon.com> wrote:
> Hi,
> 
> Please see:
> 
> https://marc.info/?l=openbsd-tech&m=164834052628572&w=2

Thanks, I didn't notice you already pointed it out.


> This code has been buggy for a long time.

Well, are there any issues that could arise from fixing it?
It's not actually validated anywhere, hotspares and new chunks
for rebuild are initialized correctly.

--
Miroslav Cimerman


> On Tue, Apr 22, 2025 at 09:01:11PM +0000, Miroslav Cimerman wrote:
> 
> > Hello,
> > 
> > In softraid, chunk metadata is incorrectly initialized.
> > The checksum is done only on the first 16 bytes.
> > 
> > It should be done on whole `struct sr_meta_chunk_invariant',
> > and after the coerced size is in place.
> > 
> > On request I can provide a simple shell script to test this,
> > I didn't want to clutter this mail.
> > 
> > --
> > Miroslav Cimerman
> > 
> > Index: sys/dev/softraid.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/softraid.c,v
> > diff -u -p -u -p -r1.433 softraid.c
> > --- sys/dev/softraid.c 8 Jan 2025 23:40:40 -0000 1.433
> > +++ sys/dev/softraid.c 22 Apr 2025 20:41:46 -0000
> > @@ -571,8 +571,6 @@ sr_meta_init(struct sr_discipline *sd, i
> > 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;
> > @@ -584,9 +582,13 @@ sr_meta_init(struct sr_discipline *sd, i
> > 
> > sm->ssdi.ssd_secsize = secsize;
> > 
> > - /* Equalize chunk sizes. /
> > - SLIST_FOREACH(chunk, cl, src_link)
> > - chunk->src_meta.scmi.scm_coerced_size = min_chunk_sz;
> > + / Equalize chunk sizes and calculate checksum. */
> > + SLIST_FOREACH(chunk, cl, src_link) {
> > + scm = &chunk->src_meta;
> > + scm->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;