Bug 205003

Summary: possible null pointer dereferences
Product: File System Reporter: p (pkoroau+bz)
Component: btrfsAssignee: BTRFS virtual assignee (fs_btrfs)
Status: RESOLVED CODE_FIX    
Severity: normal CC: dsterba, jth
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 5.4, 4.19 Subsystem:
Regression: No Bisected commit-id:

Description p 2019-09-26 06:39:11 UTC 
Found by cppcheck.


1.
In btrfsic_process_superblock() @ check-integrity.c:
null check at line 640 after dereference at line 632


2.
In btrfs_destroy_dev_replace_tgtdev() @ volumes.c:
null check at line 2310 after dereference at line 2308


3.
__readahead_hook() at reada.c passes potentially null argument eb (according to
the function doc) to these functions, which dereference eb:
btrfs_header_generation(), btrfs_header_nritems(),
btrfs_header_level().

btrfs_header_* are defined in ctree.h through BTRFS_SETGET_HEADER_FUNCS.

reada_start_machine_dev() actually calls __readahead_hook() with eb = NULL.
Comment 1 Johannes Thumshirn 2019-12-05 13:12:45 UTC 
Case Number 1 cannot happen as the only caller of btrfsic_process_superblock() is btrfsic_mount() and it allocates 'state' before calling btrfsic_process_superblock() and does proper error handling. 
Therefore we can a) remove the BUG_ON(NULL == state) completely and we can remove the local fs_info variable as well. I have patches doing both and will submit them soon.

For case number 2, it is also impossible to pass a NULL tgtdev into btrfs_destroy_dev_replace_tgtdev(), so the NULL pointer check is superfluous. I do also have a patch in the queue to remove this check.

For case number 3, this is also a false positive as we're never calling any of the btrfs_header_XXX() functions with a NULL extent_buffer. In all cases where eb could be NULL we're also setting the 'err' variable and __readahead_hook() has the following hunk in the beginning of the function:

>        /*
>         * this is the error case, the extent buffer has not been
>         * read correctly. We won't access anything from it and
>         * just cleanup our data structures. Effectively this will
>         * cut the branch below this node from read ahead.
>         */
>        if (err)
>                goto cleanup;

The patches silencing the static checkers for cases 1 and 2 can be found in:
https://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git/log/?h=btrfs-misc-fixes
Comment 2 David Sterba 2019-12-05 14:23:51 UTC 
Patches added to misc-next, thanks.