Re: disklabel,fsck_ffs: division by zero on invalid frag sizes

[kwesterback]

> On Mar 28, 2018, at 11:56 AM, Tobias Stoeckmann  wrote:
> 
>> On Wed, Mar 28, 2018 at 12:25:01PM +0200, Otto Moerbeek wrote:
>> Hmm, on what platform are you doing this? I could not reproduce your
>> problem on arm64 -current,
> 
> To sum it up: It was my fault, -current is all fine.
> 
> The issue can be reproduced with 6.2-stable amd64 and somewhere between
> 6.2-release and -current, but not with latest -current.
> 
> My i386 system uses a modified kernel (for my touchpad) and due to
> being not the fastest system, I am not always keeping the kernel up
> to date, as long as everything boots up.
> 
> Didn't expect it to be a kernel issue, so after a clean install,
> applying the diff krw@ sent me and rebuilding the kernel as well as
> disklabel and fsck_ffs, the problem went away. With stock -current
> kernel, the issue is also not reproducible.
> 
> Sorry for the noise, will give the year 2016 its diff back. ;)
> 
> 
> Tobias

Cool. Thanks for following up.

 Ken

Re: disklabel,fsck_ffs: division by zero on invalid frag sizes

[Tobias Stoeckmann]

On Wed, Mar 28, 2018 at 12:25:01PM +0200, Otto Moerbeek wrote:
> Hmm, on what platform are you doing this? I could not reproduce your
> problem on arm64 -current,

To sum it up: It was my fault, -current is all fine.

The issue can be reproduced with 6.2-stable amd64 and somewhere between
6.2-release and -current, but not with latest -current.

My i386 system uses a modified kernel (for my touchpad) and due to
being not the fastest system, I am not always keeping the kernel up
to date, as long as everything boots up.

Didn't expect it to be a kernel issue, so after a clean install,
applying the diff krw@ sent me and rebuilding the kernel as well as
disklabel and fsck_ffs, the problem went away. With stock -current
kernel, the issue is also not reproducible.

Sorry for the noise, will give the year 2016 its diff back. ;)


Tobias

Re: disklabel,fsck_ffs: division by zero on invalid frag sizes

[Otto Moerbeek]

On Tue, Mar 27, 2018 at 07:50:18PM +0200, Otto Moerbeek wrote:

> On Tue, Mar 27, 2018 at 12:46:03PM +0200, Tobias Stoeckmann wrote:
> 
> > Resending this old diff. Adjusted to apply with -current source:
> > 
> > Illegal fragmentation block sizes can trigger division by zero in the
> > disklabel and fsck_ffs tools.
> > 
> > See this sequence of commands to reproduce:
> > 
> > # dd if=/dev/zero of=nofrag.iso bs=1M count=1
> > # vnconfig vnd0 nofrag.iso
> > # disklabel -e vnd0
> > 
> > Create 'a' and set fsize = bsize = 1. A line like this one will do
> >   a: 20480  4.2BSD  1 1 1
> > 
> > # fsck_ffs vnd0a
> > ** /dev/vnd0a (vnd0a)
> > BAD SUPER BLOCK: MAGIC NUMBER WRONG
> > Floating point exception (core dumped)
> > # disklabel -E vnd0
> > Label editor (enter '?' for help at any prompt)
> > > m a
> > offset: [0]
> > size: [2048]
> > FS type: [4.2BSD]
> > Floating point exception (core dumped)
> > # vnconfig -u vnd0

Hmm, on what platform are you doing this? I could not reproduce your
problem on arm64 -current,

-Otto

> > 
> > A fragmentation (block) size smaller than a sector size is not valid
> > while using "disklabel -E", and really doesn't make sense. While
> > using "disklabel -e", not all validation checks are performed, which
> > makes it possible to enter invalid values.
> > 
> > If "disklabel -E" is used without the expert mode, fragmentation sizes
> > cannot be changed and will be just accepted from the parsed disklabel,
> > resulting in a division by zero if they are too small.
> > 
> > And the same happens in fsck_ffs. Instead of coming up with a guessed
> > value in fsck_ffs, I think it's better to simply fail and let the user
> > fix the disklabel. After all, it shouldn't be fsck_ffs's duty to fix
> > faulty values outside the filesystem.
> 
> This looks from reading the diff, but "fragmentation size" is not the
> correct term, see inline.
> 
>   -Otto
> 
> > 
> > Index: sbin/disklabel/disklabel.c
> > ===
> > RCS file: /cvs/src/sbin/disklabel/disklabel.c,v
> > retrieving revision 1.227
> > diff -u -p -u -p -r1.227 disklabel.c
> > --- sbin/disklabel/disklabel.c  25 Feb 2018 17:24:44 -  1.227
> > +++ sbin/disklabel/disklabel.c  27 Mar 2018 10:42:59 -
> > @@ -1100,9 +1100,24 @@ gottype:
> >  
> > case FS_BSDFFS:
> > NXTNUM(fsize, fsize, );
> > -   if (fsize == 0)
> > +   if (fsize < lp->d_secsize ||
> > +   (fsize % lp->d_secsize) != 0) {
> > +   warnx("line %d: "
> > +   "bad fragmentation size: %s",
> 
> Should be "fragment size"
> 
> > +   lineno, cp);
> > +   errors++;
> > break;
> > +   }
> > NXTNUM(v, v, );
> > +   if (v < fsize || (fsize != v &&
> > +   fsize * 2 != v && fsize * 4 != v &&
> > +   fsize * 8 != v)) {
> > +   warnx("line %d: "
> > +   "bad block size: %s",
> > +   lineno, cp);
> > +   errors++;
> > +   break;
> > +   }
> > pp->p_fragblock =
> > DISKLABELV1_FFS_FRAGBLOCK(fsize, v / fsize);
> > NXTNUM(pp->p_cpg, pp->p_cpg, );
> > Index: sbin/disklabel/editor.c
> > ===
> > RCS file: /cvs/src/sbin/disklabel/editor.c,v
> > retrieving revision 1.327
> > diff -u -p -u -p -r1.327 editor.c
> > --- sbin/disklabel/editor.c 8 Mar 2018 22:05:17 -   1.327
> > +++ sbin/disklabel/editor.c 27 Mar 2018 10:42:59 -
> > @@ -2069,16 +2069,16 @@ get_bsize(struct disklabel *lp, int part
> > if (pp->p_fstype != FS_BSDFFS)
> > return (0);
> >  
> > +   frag = DISKLABELV1_FFS_FRAG(pp->p_fragblock);
> > +   fsize = DISKLABELV1_FFS_FSIZE(pp->p_fragblock);
> > +
> > /* Avoid dividing by zero... */
> > -   if (pp->p_fragblock == 0)
> > -   return(1);
> > +   if (frag * fsize < lp->d_secsize)
> > +   return (1);
> >  
> > if (!expert)
> > goto align;
> >  
> > -   fsize = DISKLABELV1_FFS_FSIZE(pp->p_fragblock);
> > -   frag = DISKLABELV1_FFS_FRAG(pp->p_fragblock);
> > -
> > for (;;) {
> > ui = getuint64(lp, "block size",
> > "Size of ffs blocks. 1, 2, 4 or 8 times ffs fragment size.",
> > @@ -2119,8 +2119,7 @@ align:
> > orig_size = DL_GETPSIZE(pp);
> > orig_offset = DL_GETPOFFSET(pp);
> >  
> > -   bsize = 

Re: disklabel,fsck_ffs: division by zero on invalid frag sizes

[Otto Moerbeek]

On Tue, Mar 27, 2018 at 12:46:03PM +0200, Tobias Stoeckmann wrote:

> Resending this old diff. Adjusted to apply with -current source:
> 
> Illegal fragmentation block sizes can trigger division by zero in the
> disklabel and fsck_ffs tools.
> 
> See this sequence of commands to reproduce:
> 
> # dd if=/dev/zero of=nofrag.iso bs=1M count=1
> # vnconfig vnd0 nofrag.iso
> # disklabel -e vnd0
> 
> Create 'a' and set fsize = bsize = 1. A line like this one will do
>   a: 20480  4.2BSD  1 1 1
> 
> # fsck_ffs vnd0a
> ** /dev/vnd0a (vnd0a)
> BAD SUPER BLOCK: MAGIC NUMBER WRONG
> Floating point exception (core dumped)
> # disklabel -E vnd0
> Label editor (enter '?' for help at any prompt)
> > m a
> offset: [0]
> size: [2048]
> FS type: [4.2BSD]
> Floating point exception (core dumped)
> # vnconfig -u vnd0
> 
> A fragmentation (block) size smaller than a sector size is not valid
> while using "disklabel -E", and really doesn't make sense. While
> using "disklabel -e", not all validation checks are performed, which
> makes it possible to enter invalid values.
> 
> If "disklabel -E" is used without the expert mode, fragmentation sizes
> cannot be changed and will be just accepted from the parsed disklabel,
> resulting in a division by zero if they are too small.
> 
> And the same happens in fsck_ffs. Instead of coming up with a guessed
> value in fsck_ffs, I think it's better to simply fail and let the user
> fix the disklabel. After all, it shouldn't be fsck_ffs's duty to fix
> faulty values outside the filesystem.

This looks from reading the diff, but "fragmentation size" is not the
correct term, see inline.

-Otto

> 
> Index: sbin/disklabel/disklabel.c
> ===
> RCS file: /cvs/src/sbin/disklabel/disklabel.c,v
> retrieving revision 1.227
> diff -u -p -u -p -r1.227 disklabel.c
> --- sbin/disklabel/disklabel.c25 Feb 2018 17:24:44 -  1.227
> +++ sbin/disklabel/disklabel.c27 Mar 2018 10:42:59 -
> @@ -1100,9 +1100,24 @@ gottype:
>  
>   case FS_BSDFFS:
>   NXTNUM(fsize, fsize, );
> - if (fsize == 0)
> + if (fsize < lp->d_secsize ||
> + (fsize % lp->d_secsize) != 0) {
> + warnx("line %d: "
> + "bad fragmentation size: %s",

Should be "fragment size"

> + lineno, cp);
> + errors++;
>   break;
> + }
>   NXTNUM(v, v, );
> + if (v < fsize || (fsize != v &&
> + fsize * 2 != v && fsize * 4 != v &&
> + fsize * 8 != v)) {
> + warnx("line %d: "
> + "bad block size: %s",
> + lineno, cp);
> + errors++;
> + break;
> + }
>   pp->p_fragblock =
>   DISKLABELV1_FFS_FRAGBLOCK(fsize, v / fsize);
>   NXTNUM(pp->p_cpg, pp->p_cpg, );
> Index: sbin/disklabel/editor.c
> ===
> RCS file: /cvs/src/sbin/disklabel/editor.c,v
> retrieving revision 1.327
> diff -u -p -u -p -r1.327 editor.c
> --- sbin/disklabel/editor.c   8 Mar 2018 22:05:17 -   1.327
> +++ sbin/disklabel/editor.c   27 Mar 2018 10:42:59 -
> @@ -2069,16 +2069,16 @@ get_bsize(struct disklabel *lp, int part
>   if (pp->p_fstype != FS_BSDFFS)
>   return (0);
>  
> + frag = DISKLABELV1_FFS_FRAG(pp->p_fragblock);
> + fsize = DISKLABELV1_FFS_FSIZE(pp->p_fragblock);
> +
>   /* Avoid dividing by zero... */
> - if (pp->p_fragblock == 0)
> - return(1);
> + if (frag * fsize < lp->d_secsize)
> + return (1);
>  
>   if (!expert)
>   goto align;
>  
> - fsize = DISKLABELV1_FFS_FSIZE(pp->p_fragblock);
> - frag = DISKLABELV1_FFS_FRAG(pp->p_fragblock);
> -
>   for (;;) {
>   ui = getuint64(lp, "block size",
>   "Size of ffs blocks. 1, 2, 4 or 8 times ffs fragment size.",
> @@ -2119,8 +2119,7 @@ align:
>   orig_size = DL_GETPSIZE(pp);
>   orig_offset = DL_GETPOFFSET(pp);
>  
> - bsize = (DISKLABELV1_FFS_FRAG(pp->p_fragblock) *
> - DISKLABELV1_FFS_FSIZE(pp->p_fragblock)) / lp->d_secsize;
> + bsize = (frag * fsize) / lp->d_secsize;
>   if (DL_GETPOFFSET(pp) != starting_sector) {
>   /* Can't change offset of first partition. */
>   adj = bsize - (DL_GETPOFFSET(pp) %