[lvc-project] [PATCH 5.10/5.15] jfs: add check if log->bdev is NULL in lbmStartIO()

[Fedor Pchelkin]

Hello,

On 24/01/12 07:50PM, Mikhail Ukhin wrote:
> Fuzzing of 5.10 stable branch shows NULL pointer dereference happens in 
> lbmStartIO() on log->bdev pointer. The reason for bdev being NULL is the 
> JFS_NOINTEGRITY flag is set on mount of this fs. When this flag is enabled,
> it results in the open_dummy_log function being called, which initializes a
> new dummy_log, but does not assign a value to bdev.
>

Thank you for the patch and work on this bug!

Frankly speaking, I don't understand why Greg KH has refused to take the
patch and don't know how the filesystems are supposed to be marked
BROKEN. JFS is not an actively developed filesystem and it seems to be
only getting bugfixes (usually backported) and generic API changes
(usually not backported) for quite a long time.

I just wonder when this bug was introduced? Does it actually go back all
the time to 1da177e4c3f4 ("Linux-2.6.12-rc2")? If yes, then this
filesystem was actually unusable with JFS_NOINTEGRITY flag before v5.18.
It is weird that nobody has noticed that and fixed before.

I see your patch applied in LVC-supported branches so it's fine to just
leave it there. If Greg KH for some reason doesn't want it applied to
Stable Group supported branches - well, okay...

> The error is fixed in 5.18 by commit
> 07888c665b405b1cd3577ddebfeb74f4717a84c4.
> Backport of this commit is too intrusive, so it is more reasonable to apply
> a small patch to fix this issue.
> 
> Found by Linux Verification Center (linuxtesting.org) with syzkaller.
> 
> Signed-off-by: Mikhail Ukhin <mish.uxin2012 at yandex.ru>
> Signed-off-by: Mikhail Ivanov <iwanov-23 at bk.ru>
> Signed-off-by: Pavel Koshutin <koshutin.pavel at yandex.ru>
> Signed-off-by: Artem Sadovnikov <ancowi69 at gmail.com>
> ---
>  fs/jfs/jfs_logmgr.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
> index 78fd136ac13b..d6f0fea96ba1 100644
> --- a/fs/jfs/jfs_logmgr.c
> +++ b/fs/jfs/jfs_logmgr.c
> @@ -1983,7 +1983,8 @@ static int lbmRead(struct jfs_log * log, int pn, struct lbuf ** bpp)
>  	bio = bio_alloc(GFP_NOFS, 1);
>  
>  	bio->bi_iter.bi_sector = bp->l_blkno << (log->l2bsize - 9);
> -	bio_set_dev(bio, log->bdev);
> +	if (log->bdev != NULL)

Just a minor question. Maybe use `if (!log->no_integrity)` here for
indication that bio_set_dev() should be avoided only in a no_integrity
case?

> +		bio_set_dev(bio, log->bdev);
>  
>  	bio_add_page(bio, bp->l_page, LOGPSIZE, bp->l_offset);
>  	BUG_ON(bio->bi_iter.bi_size != LOGPSIZE);
> @@ -2127,7 +2128,8 @@ static void lbmStartIO(struct lbuf * bp)
>  
>  	bio = bio_alloc(GFP_NOFS, 1);
>  	bio->bi_iter.bi_sector = bp->l_blkno << (log->l2bsize - 9);
> -	bio_set_dev(bio, log->bdev);
> +	if (log->bdev != NULL)

And here, too.

> +		bio_set_dev(bio, log->bdev);
>  
>  	bio_add_page(bio, bp->l_page, LOGPSIZE, bp->l_offset);
>  	BUG_ON(bio->bi_iter.bi_size != LOGPSIZE);
> -- 
> 2.25.1
> 

By the way, there was a similar bug [1] introduced in a recent kernel
release (v5.10 is not exposed to), and the reason looks the same.

[1]: https://syzkaller.appspot.com/bug?extid=23bc20037854bb335d59