linux-ext4 - Re: [bug report] ext4: xattr-in-inode support

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] [day] [month] [year] [list]

Date:   Fri, 13 Jul 2018 22:47:27 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Dan Carpenter <dan.carpenter@...cle.com>,
        "Theodore Y. Ts'o" <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [bug report] ext4: xattr-in-inode support

On Jul 12, 2018, at 7:30 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> 
> Hello Andreas Dilger,
> 
> The patch e50e5129f384: "ext4: xattr-in-inode support" from Jun 21,
> 2017, leads to the following static checker warning:
> 
> 	fs/ext4/xattr.c:1393 ext4_xattr_inode_write()
> 	warn: 'bh' can also be NULL

Initially I thought "IS_ERR_OR_NULL()" or a separate NULL check was
in order.

Looking at this more closely, it doesn't appear that this can actually
happen. Immediately before this code, there is a loop that is calling
ext4_map_blocks() to allocate all of the blocks, and this is only
fetching the bh references again, so the "err == 0" case (no block)
therein cannot be hit.

It is done this way so that all of the allocations can be done at once,
rather than interleaving 1-block allocations and writes.

That said, this is definitely not easily determined by static code
analysis, nor necessarily by the reader.

Possible solutions:
1) add a comment here explaining why bh can't be NULL.  In theory, it
   might be possible for the block to be deallocated before the call
   to ext4_getblk() (e.g. corruption in the small window), but unlikely.
2) add a comment to tell the static analyzer to ignore this case (not
   sure if this is possible or not, depends on the tool)
3) add "map_flags = EXT4_GET_BLOCKS_CREATE" argument to ext4_getblk(),
   even though it would be redundant, so that the static checker cannot
   get into the "return NULL" condition.  It would also potentially
   avoid the unlikely oops if the filesystem state were corrupted at
   the wrong time, though I'm not sure if that is good or bad.
4) have static analysis track filesystem state across ext4_map_blocks()
   and ext4_getblk(), but very unlikely (though there *are* some tools
   that will do this kind of thing, but they are very different beasts).

I'd be inclined to do #3 and add a comment that EXT4_GET_BLOCKS_CREATE
is just for cosmetic/static checker reasons and isn't actually needed.

Ted, do you have an opinion on this?

Cheers, Andreas

> fs/ext4/xattr.c
>  1380          block = 0;
>  1381          while (wsize < bufsize) {
>  1382                  if (bh != NULL)
>  1383                          brelse(bh);
>  1384                  csize = (bufsize - wsize) > blocksize ? blocksize :
>  1385                                                                  bufsize - wsize;
>  1386                  bh = ext4_getblk(handle, ea_inode, block, 0);
>  1387                  if (IS_ERR(bh))
>  1388                          return PTR_ERR(bh);
>  1389                  ret = ext4_journal_get_write_access(handle, bh);
>  1390                  if (ret)
>  1391                          goto out;
>  1392
>  1393                  memcpy(bh->b_data, buf, csize);
> 
> I don't know the code well enought to say if it's an issue.  I really
> wish there were comments when functions return a mix of error pointers
> and NULL.
> 
>  1394                  set_buffer_uptodate(bh);
>  1395                  ext4_handle_dirty_metadata(handle, ea_inode, bh);
>  1396
>  1397                  buf += csize;
>  1398                  wsize += csize;
>  1399                  block += 1;
>  1400          }
> 
> regards,
> dan carpenter

Cheers, Andreas

Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)