linux-kernel - Re: [PATCH 09/15] udf: avoid gcc-10 zero-length-bounds warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 May 2020 22:48:43 +0200
From:   Jan Kara <jack@...e.cz>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Pali Rohár <pali@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Steven J. Magnani" <steve@...idescorp.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH 09/15] udf: avoid gcc-10 zero-length-bounds warnings

On Fri 01-05-20 22:30:27, Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 11:54 PM Pali Rohár <pali@...nel.org> wrote:
> > > @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
> > >       uint8_t                 logicalVolContentsUse[32];
> > >       __le32                  numOfPartitions;
> > >       __le32                  lengthOfImpUse;
> > > -     __le32                  freeSpaceTable[0];
> > >       __le32                  sizeTable[0];
> > >       uint8_t                 impUse[0];
> > > +     __le32                  freeSpaceTable[];
> >
> > Please do not change order of members in these structures. Order is
> > strictly defined by ECMA 167 standard and changing them you would just
> > confuse reader. In LVID is free space table before size table.
> 
> Ok
> 
> > If you do not like GNU C extension for zero-length arrays then just
> > replace it by standard C99 flexible arrays. I think that there is no
> > reason to not use standard C99 language constructions, just nobody had
> > motivation or time to change (working) code.
> 
> No, the problem is that only the last member can be a flexible array,
> so when impUse[] is the last member, freeSpaceTable has to be a zero
> length array.
> 
> []> Also this file is semi-synchronized with udftools project in which I
> > already replaced all GNU C zero-length arrays by C99 flexible arrays.
> >
> > You can take inspiration what I did with logicalVolIntegrityDesc:
> > https://github.com/pali/udftools/commit/f851d84478ce881d516a76018745fa163f803880#diff-1e1a5b89f620d380f22b973f9449aeaeL381-R384
> 
> Right, this is likely the best workaround.
> 
> > Anyway, if you have a better idea what to do with such on-disk structure
> > and how to represent it in C struct syntax, let me know as it could be
> > updated also in udftools project.
> 
> The trick I used for impUse[] would also work for freeSpaceTable[] to avoid
> the gcc warning, it's still not great, but maybe you like this better:

I like Pali's version somewhat better because whenever I look at several
(obviously flexible) arrays in one struct, I start wondering what's going
on. So let's not define members of struct whose offset we actually don't
know (and thus we cannot sanely use them anyway).

								Honza
 
> arnd@...eadripper:~/arm-soc$ git diff
> diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
> index 02f03fadb75b..666d022eb00b 100644
> --- a/fs/udf/balloc.c
> +++ b/fs/udf/balloc.c
> @@ -98,7 +98,7 @@ static void udf_add_free_space(struct super_block
> *sb, u16 partition, u32 cnt)
>                 return;
> 
>         lvid = (struct logicalVolIntegrityDesc *)sbi->s_lvid_bh->b_data;
> -       le32_add_cpu(&lvid->freeSpaceTable[partition], cnt);
> +       le32_add_cpu(lvid->freeSpaceTable + partition, cnt);
>         udf_updated_lvid(sb);
>  }
> 
> diff --git a/fs/udf/ecma_167.h b/fs/udf/ecma_167.h
> index 14ffe27342bc..215d97d7edc4 100644
> --- a/fs/udf/ecma_167.h
> +++ b/fs/udf/ecma_167.h
> @@ -360,9 +360,9 @@ struct logicalVolIntegrityDesc {
>         uint8_t                 logicalVolContentsUse[32];
>         __le32                  numOfPartitions;
>         __le32                  lengthOfImpUse;
>         __le32                  freeSpaceTable[0];
>         __le32                  sizeTable[0];
> -       uint8_t                 impUse[0];
> +       uint8_t                 impUse[];
>  } __packed;
> 
>  /* Integrity Type (ECMA 167r3 3/10.10.3) */
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 379867888c36..a1fc51c2261e 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -2517,8 +2517,8 @@ static unsigned int udf_count_free(struct super_block *sb)
>                         (struct logicalVolIntegrityDesc *)
>                         sbi->s_lvid_bh->b_data;
>                 if (le32_to_cpu(lvid->numOfPartitions) > part) {
> -                       accum = le32_to_cpu(
> -                                       lvid->freeSpaceTable[part]);
> +                       accum = le32_to_cpup(
> +                                       (lvid->freeSpaceTable + part));
>                         if (accum == 0xFFFFFFFF)
>                                 accum = 0;
>                 }
> 
> 
> 
> This version could easily be backported to stable kernels to let them be
> compiled with gcc-10, and then synchronizing with the udftools version of
> the header needs additional changes on top, which do not need to be
> backported.
> 
>        Arnd
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR