another possible integer truncation in xfs

another possible integer truncation in xfs

[Markus Stockhausen]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

Hi Christoph,

out of curiosity I looked for other use cases of min_t in xfs. At least 
until 4.12 there is a similar constellation in xfs_dir2_leaf_readbuf:

  if (trim_map) {
    mip->map_blocks -= geo->fsbcount;
    /*
     * Loop to get rid of the extents for the
     * directory block.
     */
    for (i = geo->fsbcount; i > 0; ) {
      j = min_t(int, map->br_blockcount, i);
      map->br_blockcount -= j;
      map->br_startblock += j;
      map->br_startoff += j;

The loop could go havoc if map->br_blockcount is larger than 
2G. If you think it could classify for stable feel free to add it too.

Best regards.

Markus
=

[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]

****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.

Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.

Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln

Vorstand:
Kadir Akin
Dr. Michael Höhnerbach

Vorsitzender des Aufsichtsrates:
Hans Kristian Langva

Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497

This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.

e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.

Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln

executive board:
Kadir Akin
Dr. Michael Höhnerbach

President of the supervisory board:
Hans Kristian Langva

Registry office: district court Cologne
Register number: HRB 52 497

****************************************************************************

Re: another possible integer truncation in xfs

[Christoph Hellwig]

On Mon, Aug 21, 2017 at 08:01:03AM +0000, Markus Stockhausen wrote:
> Hi Christoph,
> 
> out of curiosity I looked for other use cases of min_t in xfs. At least 
> until 4.12 there is a similar constellation in xfs_dir2_leaf_readbuf:
> 
>   if (trim_map) {
>     mip->map_blocks -= geo->fsbcount;
>     /*
>      * Loop to get rid of the extents for the
>      * directory block.
>      */
>     for (i = geo->fsbcount; i > 0; ) {
>       j = min_t(int, map->br_blockcount, i);
>       map->br_blockcount -= j;
>       map->br_startblock += j;
>       map->br_startoff += j;
> 
> The loop could go havoc if map->br_blockcount is larger than 
> 2G. If you think it could classify for stable feel free to add it too.

I don't think it has a chance to be larger in practice, but we should
fix it anyway.  I'll prepare a patch.

Thanks for spotting this!

Re: another possible integer truncation in xfs

[Darrick J. Wong]

On Mon, Aug 21, 2017 at 10:16:03AM +0200, Christoph Hellwig wrote:
> On Mon, Aug 21, 2017 at 08:01:03AM +0000, Markus Stockhausen wrote:
> > Hi Christoph,
> > 
> > out of curiosity I looked for other use cases of min_t in xfs. At least 
> > until 4.12 there is a similar constellation in xfs_dir2_leaf_readbuf:
> > 
> >   if (trim_map) {
> >     mip->map_blocks -= geo->fsbcount;
> >     /*
> >      * Loop to get rid of the extents for the
> >      * directory block.
> >      */
> >     for (i = geo->fsbcount; i > 0; ) {
> >       j = min_t(int, map->br_blockcount, i);
> >       map->br_blockcount -= j;
> >       map->br_startblock += j;
> >       map->br_startoff += j;
> > 
> > The loop could go havoc if map->br_blockcount is larger than 
> > 2G. If you think it could classify for stable feel free to add it too.

"2G"... are you concerned about an integer overflow if map->br_blockcount
is a value larger than 2147483647, or if *map itself represents an
extent longer than 2GiB?  (I'm pretty sure you're talking about the
first scenario, but the units here are ambiguous.)

I /think/ the correct answer here is that file extent records can't ever
be longer than 2^20 blocks so this min_t ought to be fine.

--D

> I don't think it has a chance to be larger in practice, but we should
> fix it anyway.  I'll prepare a patch.
> 
> Thanks for spotting this!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

AW: another possible integer truncation in xfs

[Markus Stockhausen]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

> Von: Darrick J. Wong [darrick.wong@oracle.com]
> Gesendet: Dienstag, 22. August 2017 18:41
> An: Christoph Hellwig
> Cc: Markus Stockhausen; pmenzel@molgen.mpg.de; linux-xfs@vger.kernel.org
> Betreff: Re: another possible integer truncation in xfs
> 
> On Mon, Aug 21, 2017 at 10:16:03AM +0200, Christoph Hellwig wrote:
> > On Mon, Aug 21, 2017 at 08:01:03AM +0000, Markus Stockhausen wrote:
> > > Hi Christoph,
> > >
> > > out of curiosity I looked for other use cases of min_t in xfs. At least
> > > until 4.12 there is a similar constellation in xfs_dir2_leaf_readbuf:
> > >
> > >   if (trim_map) {
> > >     mip->map_blocks -= geo->fsbcount;
> > >     /*
> > >      * Loop to get rid of the extents for the
> > >      * directory block.
> > >      */
> > >     for (i = geo->fsbcount; i > 0; ) {
> > >       j = min_t(int, map->br_blockcount, i);
> > >       map->br_blockcount -= j;
> > >       map->br_startblock += j;
> > >       map->br_startoff += j;
> > >
> > > The loop could go havoc if map->br_blockcount is larger than
> > > 2G. If you think it could classify for stable feel free to add it too.
> 
> "2G"... are you concerned about an integer overflow if map->br_blockcount
> is a value larger than 2147483647, or if *map itself represents an
> extent longer than 2GiB?  (I'm pretty sure you're talking about the
> first scenario, but the units here are ambiguous.)
> 
> I /think/ the correct answer here is that file extent records can't ever
> be longer than 2^20 blocks so this min_t ought to be fine.

Hi Darrick,

It was just the same educated guess as the first spotting. 64 bit
variables minified by their lower 32 bits. Thus truncation "could" 
lead to strange side effects. If it cannot happen - sorry for the 
noise.

Markus

> --D
> 
> > I don't think it has a chance to be larger in practice, but we should
> > fix it anyway.  I'll prepare a patch.
> >
> > Thanks for spotting this!
> > --
=

[-- Attachment #2: InterScan_Disclaimer.txt --]
[-- Type: text/plain, Size: 1650 bytes --]

****************************************************************************
Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte
Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail
irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte
Weitergabe dieser Mail ist nicht gestattet.

Über das Internet versandte E-Mails können unter fremden Namen erstellt oder
manipuliert werden. Deshalb ist diese als E-Mail verschickte Nachricht keine
rechtsverbindliche Willenserklärung.

Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln

Vorstand:
Kadir Akin
Dr. Michael Höhnerbach

Vorsitzender des Aufsichtsrates:
Hans Kristian Langva

Registergericht: Amtsgericht Köln
Registernummer: HRB 52 497

This e-mail may contain confidential and/or privileged information. If you
are not the intended recipient (or have received this e-mail in error)
please notify the sender immediately and destroy this e-mail. Any
unauthorized copying, disclosure or distribution of the material in this
e-mail is strictly forbidden.

e-mails sent over the internet may have been written under a wrong name or
been manipulated. That is why this message sent as an e-mail is not a
legally binding declaration of intention.

Collogia
Unternehmensberatung AG
Ubierring 11
D-50678 Köln

executive board:
Kadir Akin
Dr. Michael Höhnerbach

President of the supervisory board:
Hans Kristian Langva

Registry office: district court Cologne
Register number: HRB 52 497

****************************************************************************