linux-kernel - Re: [PATCH 3/3] udf: refactor inode

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]

Message-ID: <20240920160037.i4nla2ccn7rtmb7n@quack3>
Date: Fri, 20 Sep 2024 18:00:37 +0200
From: Jan Kara <jack@...e.cz>
To: Zhao Mengmeng <zhaomzhao@....com>
Cc: jack@...e.com, zhaomengmeng@...inos.cn, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] udf: refactor inode_bmap() to handle error

On Wed 18-09-24 17:36:34, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@...inos.cn>
> 
> Same as udf_current_aext(), take pointer to etype to store the extent
> type, while return 0 for success and <0 on error. On situations like
> ftruncate, udf_extend_file() can detect errors and bail out early
> without resorting to checking for particular offsets and
> assuming internal behavior of these functions.
> 
> Reported-by: syzbot+7a4842f0b1801230a989@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7a4842f0b1801230a989
> Tested-by: syzbot+7a4842f0b1801230a989@...kaller.appspotmail.com
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@...inos.cn>

Overall looks good. I have a few comments below.

> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index ba980ce5e13a..f1b8f0a0d202 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -418,10 +418,11 @@ static int udf_map_block(struct inode *inode, struct udf_map_rq *map)
>  		uint32_t elen;
>  		sector_t offset;
>  		struct extent_position epos = {};
> +		int8_t etype;
>  
>  		down_read(&iinfo->i_data_sem);
> -		if (inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset)
> -				== (EXT_RECORDED_ALLOCATED >> 30)) {
> +		inode_bmap(inode, map->lblk, &epos, &eloc, &elen, &offset, &etype);

Here we should be checking for error and returning it...

> +		if (etype == (EXT_RECORDED_ALLOCATED >> 30)) {
>  			map->pblk = udf_get_lb_pblock(inode->i_sb, &eloc,
>  							offset);
>  			map->oflags |= UDF_BLK_MAPPED;
> @@ -660,8 +661,10 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>  	 */
>  	udf_discard_prealloc(inode);
>  
> -	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
> -	within_last_ext = (etype != -1);
> +	err = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset, &etype);
> +	if (err < 0 && err != -ENODATA)
> +		goto out;
> +	within_last_ext = (!err);
>  	/* We don't expect extents past EOF... */
>  	WARN_ON_ONCE(within_last_ext &&
>  		     elen > ((loff_t)offset + 1) << inode->i_blkbits);
> @@ -2363,14 +2366,19 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>  	return (elen >> 30);
>  }
>  
> -int8_t inode_bmap(struct inode *inode, sector_t block,
> -		  struct extent_position *pos, struct kernel_lb_addr *eloc,
> -		  uint32_t *elen, sector_t *offset)
> +/*
> + * return 0 when iudf_next_aext() loop success.
> + * return err < 0 and err != -ENODATA indicates error.
> + * return err == -ENODATA indicates hit EOF.
> + */
> +int inode_bmap(struct inode *inode, sector_t block, struct extent_position *pos,
> +	       struct kernel_lb_addr *eloc, uint32_t *elen, sector_t *offset,
> +	       int8_t *etype)
>  {
>  	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
>  	loff_t lbcount = 0, bcount = (loff_t) block << blocksize_bits;
> -	int8_t etype;
>  	struct udf_inode_info *iinfo;
> +	int err = 0;
>  
>  	iinfo = UDF_I(inode);
>  	if (!udf_read_extent_cache(inode, bcount, &lbcount, pos)) {
> @@ -2380,10 +2388,11 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>  	}
>  	*elen = 0;
>  	do {
> -		if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
> +		err = udf_next_aext(inode, pos, eloc, elen, etype, 1);
> +		if (err < 0) {

OK, you've added the error handling in this patch. That is good. But still
we should be setting offset and i_lenExtents only in -ENODATA case.
Otherwise we just want to return the error.

>  			*offset = (bcount - lbcount) >> blocksize_bits;
>  			iinfo->i_lenExtents = lbcount;
> -			return -1;
> +			return err;
>  		}

...

> @@ -196,10 +197,10 @@ int udf_truncate_extents(struct inode *inode)
>  	else
>  		BUG();
>  
> -	etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
> +	err = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset, &etype);
>  	byte_offset = (offset << sb->s_blocksize_bits) +
>  		(inode->i_size & (sb->s_blocksize - 1));
> -	if (etype == -1) {
> +	if (err < 0) {

Here we should be distinguisting -ENODATA (WARN and return 0) and other
error (which we need to return).

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR