All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] quota: Handle corrupted quota file better
@ 2020-11-02 17:27 Jan Kara
  2020-11-02 17:27 ` [PATCH 1/2] quota: Don't overflow quota file offsets Jan Kara
  2020-11-02 17:27 ` [PATCH 2/2] quota: Sanity-check quota file headers on load Jan Kara
  0 siblings, 2 replies; 6+ messages in thread
From: Jan Kara @ 2020-11-02 17:27 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Jan Kara

Hello,

syzbot has spotted that quota code does not handle corrupted quota files quite
well. Let's add some sanity checks to avoid kernel crashes when accessing
corrupted quota files.

								Honza

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] quota: Don't overflow quota file offsets
  2020-11-02 17:27 [PATCH 0/2] quota: Handle corrupted quota file better Jan Kara
@ 2020-11-02 17:27 ` Jan Kara
  2020-11-02 21:30   ` Andreas Dilger
  2020-11-02 17:27 ` [PATCH 2/2] quota: Sanity-check quota file headers on load Jan Kara
  1 sibling, 1 reply; 6+ messages in thread
From: Jan Kara @ 2020-11-02 17:27 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Jan Kara

The on-disk quota format supports quota files with upto 2^32 blocks. Be
careful when computing quota file offsets in the quota files from block
numbers as they can overflow 32-bit types. Since quota files larger than
4GB would require ~26 millions of quota users, this is mostly a
theoretical concern now but better be careful, fuzzers would find the
problem sooner or later anyway...

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/quota_tree.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
index a6f856f341dc..c5562c871c8b 100644
--- a/fs/quota/quota_tree.c
+++ b/fs/quota/quota_tree.c
@@ -62,7 +62,7 @@ static ssize_t read_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
 
 	memset(buf, 0, info->dqi_usable_bs);
 	return sb->s_op->quota_read(sb, info->dqi_type, buf,
-	       info->dqi_usable_bs, blk << info->dqi_blocksize_bits);
+	       info->dqi_usable_bs, (loff_t)blk << info->dqi_blocksize_bits);
 }
 
 static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
@@ -71,7 +71,7 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
 	ssize_t ret;
 
 	ret = sb->s_op->quota_write(sb, info->dqi_type, buf,
-	       info->dqi_usable_bs, blk << info->dqi_blocksize_bits);
+	       info->dqi_usable_bs, (loff_t)blk << info->dqi_blocksize_bits);
 	if (ret != info->dqi_usable_bs) {
 		quota_error(sb, "dquota write failed");
 		if (ret >= 0)
@@ -284,7 +284,7 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
 			    blk);
 		goto out_buf;
 	}
-	dquot->dq_off = (blk << info->dqi_blocksize_bits) +
+	dquot->dq_off = ((loff_t)blk << info->dqi_blocksize_bits) +
 			sizeof(struct qt_disk_dqdbheader) +
 			i * info->dqi_entry_size;
 	kfree(buf);
@@ -559,7 +559,7 @@ static loff_t find_block_dqentry(struct qtree_mem_dqinfo *info,
 		ret = -EIO;
 		goto out_buf;
 	} else {
-		ret = (blk << info->dqi_blocksize_bits) + sizeof(struct
+		ret = ((loff_t)blk << info->dqi_blocksize_bits) + sizeof(struct
 		  qt_disk_dqdbheader) + i * info->dqi_entry_size;
 	}
 out_buf:
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] quota: Sanity-check quota file headers on load
  2020-11-02 17:27 [PATCH 0/2] quota: Handle corrupted quota file better Jan Kara
  2020-11-02 17:27 ` [PATCH 1/2] quota: Don't overflow quota file offsets Jan Kara
@ 2020-11-02 17:27 ` Jan Kara
  2020-11-02 21:32   ` Andreas Dilger
  1 sibling, 1 reply; 6+ messages in thread
From: Jan Kara @ 2020-11-02 17:27 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Jan Kara, stable

Perform basic sanity checks of quota headers to avoid kernel crashes on
corrupted quota files.

CC: stable@vger.kernel.org
Reported-by: syzbot+f816042a7ae2225f25ba@syzkaller.appspotmail.com
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/quota_v2.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/quota/quota_v2.c b/fs/quota/quota_v2.c
index e69a2bfdd81c..c21106557a37 100644
--- a/fs/quota/quota_v2.c
+++ b/fs/quota/quota_v2.c
@@ -157,6 +157,25 @@ static int v2_read_file_info(struct super_block *sb, int type)
 		qinfo->dqi_entry_size = sizeof(struct v2r1_disk_dqblk);
 		qinfo->dqi_ops = &v2r1_qtree_ops;
 	}
+	ret = -EUCLEAN;
+	/* Some sanity checks of the read headers... */
+	if ((loff_t)qinfo->dqi_blocks << qinfo->dqi_blocksize_bits >
+	    i_size_read(sb_dqopt(sb)->files[type])) {
+		quota_error(sb, "Number of blocks too big for quota file size (%llu > %llu).",
+		    (loff_t)qinfo->dqi_blocks << qinfo->dqi_blocksize_bits,
+		    i_size_read(sb_dqopt(sb)->files[type]));
+		goto out;
+	}
+	if (qinfo->dqi_free_blk >= qinfo->dqi_blocks) {
+		quota_error(sb, "Free block number too big (%u >= %u).",
+			    qinfo->dqi_free_blk, qinfo->dqi_blocks);
+		goto out;
+	}
+	if (qinfo->dqi_free_entry >= qinfo->dqi_blocks) {
+		quota_error(sb, "Block with free entry too big (%u >= %u).",
+			    qinfo->dqi_free_entry, qinfo->dqi_blocks);
+		goto out;
+	}
 	ret = 0;
 out:
 	up_read(&dqopt->dqio_sem);
-- 
2.16.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] quota: Don't overflow quota file offsets
  2020-11-02 17:27 ` [PATCH 1/2] quota: Don't overflow quota file offsets Jan Kara
@ 2020-11-02 21:30   ` Andreas Dilger
  2020-11-03  8:57     ` Jan Kara
  0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2020-11-02 21:30 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-fsdevel

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

On Nov 2, 2020, at 10:27 AM, Jan Kara <jack@suse.cz> wrote:
> 
> The on-disk quota format supports quota files with upto 2^32 blocks. Be
> careful when computing quota file offsets in the quota files from block
> numbers as they can overflow 32-bit types. Since quota files larger than
> 4GB would require ~26 millions of quota users, this is mostly a
> theoretical concern now but better be careful, fuzzers would find the
> problem sooner or later anyway...
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Out of curiosity, is this 26 million *quota entries*, or is it just a UID
larger than 26M?  At one point the quota files were sparse and indexed by
the UID, but I guess very file name "quota tree" means this is not correct.
Is there some document/comment that describes the on-disk quota file format?

In any case, the change makes sense regardless, since ->quota_read() takes
loff_t for the offset.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/quota/quota_tree.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> index a6f856f341dc..c5562c871c8b 100644
> --- a/fs/quota/quota_tree.c
> +++ b/fs/quota/quota_tree.c
> @@ -62,7 +62,7 @@ static ssize_t read_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> 
> 	memset(buf, 0, info->dqi_usable_bs);
> 	return sb->s_op->quota_read(sb, info->dqi_type, buf,
> -	       info->dqi_usable_bs, blk << info->dqi_blocksize_bits);
> +	       info->dqi_usable_bs, (loff_t)blk << info->dqi_blocksize_bits);
> }
> 
> static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> @@ -71,7 +71,7 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> 	ssize_t ret;
> 
> 	ret = sb->s_op->quota_write(sb, info->dqi_type, buf,
> -	       info->dqi_usable_bs, blk << info->dqi_blocksize_bits);
> +	       info->dqi_usable_bs, (loff_t)blk << info->dqi_blocksize_bits);
> 	if (ret != info->dqi_usable_bs) {
> 		quota_error(sb, "dquota write failed");
> 		if (ret >= 0)
> @@ -284,7 +284,7 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
> 			    blk);
> 		goto out_buf;
> 	}
> -	dquot->dq_off = (blk << info->dqi_blocksize_bits) +
> +	dquot->dq_off = ((loff_t)blk << info->dqi_blocksize_bits) +
> 			sizeof(struct qt_disk_dqdbheader) +
> 			i * info->dqi_entry_size;
> 	kfree(buf);
> @@ -559,7 +559,7 @@ static loff_t find_block_dqentry(struct qtree_mem_dqinfo *info,
> 		ret = -EIO;
> 		goto out_buf;
> 	} else {
> -		ret = (blk << info->dqi_blocksize_bits) + sizeof(struct
> +		ret = ((loff_t)blk << info->dqi_blocksize_bits) + sizeof(struct
> 		  qt_disk_dqdbheader) + i * info->dqi_entry_size;
> 	}
> out_buf:
> --
> 2.16.4
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] quota: Sanity-check quota file headers on load
  2020-11-02 17:27 ` [PATCH 2/2] quota: Sanity-check quota file headers on load Jan Kara
@ 2020-11-02 21:32   ` Andreas Dilger
  0 siblings, 0 replies; 6+ messages in thread
From: Andreas Dilger @ 2020-11-02 21:32 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-fsdevel, stable

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

On Nov 2, 2020, at 10:27 AM, Jan Kara <jack@suse.cz> wrote:
> 
> Perform basic sanity checks of quota headers to avoid kernel crashes on
> corrupted quota files.
> 
> CC: stable@vger.kernel.org
> Reported-by: syzbot+f816042a7ae2225f25ba@syzkaller.appspotmail.com
> Signed-off-by: Jan Kara <jack@suse.cz>

Looks reasonable.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/quota/quota_v2.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
> 
> diff --git a/fs/quota/quota_v2.c b/fs/quota/quota_v2.c
> index e69a2bfdd81c..c21106557a37 100644
> --- a/fs/quota/quota_v2.c
> +++ b/fs/quota/quota_v2.c
> @@ -157,6 +157,25 @@ static int v2_read_file_info(struct super_block *sb, int type)
> 		qinfo->dqi_entry_size = sizeof(struct v2r1_disk_dqblk);
> 		qinfo->dqi_ops = &v2r1_qtree_ops;
> 	}
> +	ret = -EUCLEAN;
> +	/* Some sanity checks of the read headers... */
> +	if ((loff_t)qinfo->dqi_blocks << qinfo->dqi_blocksize_bits >
> +	    i_size_read(sb_dqopt(sb)->files[type])) {
> +		quota_error(sb, "Number of blocks too big for quota file size (%llu > %llu).",
> +		    (loff_t)qinfo->dqi_blocks << qinfo->dqi_blocksize_bits,
> +		    i_size_read(sb_dqopt(sb)->files[type]));
> +		goto out;
> +	}
> +	if (qinfo->dqi_free_blk >= qinfo->dqi_blocks) {
> +		quota_error(sb, "Free block number too big (%u >= %u).",
> +			    qinfo->dqi_free_blk, qinfo->dqi_blocks);
> +		goto out;
> +	}
> +	if (qinfo->dqi_free_entry >= qinfo->dqi_blocks) {
> +		quota_error(sb, "Block with free entry too big (%u >= %u).",
> +			    qinfo->dqi_free_entry, qinfo->dqi_blocks);
> +		goto out;
> +	}
> 	ret = 0;
> out:
> 	up_read(&dqopt->dqio_sem);
> --
> 2.16.4
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] quota: Don't overflow quota file offsets
  2020-11-02 21:30   ` Andreas Dilger
@ 2020-11-03  8:57     ` Jan Kara
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2020-11-03  8:57 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Jan Kara, linux-fsdevel

On Mon 02-11-20 14:30:37, Andreas Dilger wrote:
> On Nov 2, 2020, at 10:27 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > The on-disk quota format supports quota files with upto 2^32 blocks. Be
> > careful when computing quota file offsets in the quota files from block
> > numbers as they can overflow 32-bit types. Since quota files larger than
> > 4GB would require ~26 millions of quota users, this is mostly a
> > theoretical concern now but better be careful, fuzzers would find the
> > problem sooner or later anyway...
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Out of curiosity, is this 26 million *quota entries*, or is it just a UID
> larger than 26M?  At one point the quota files were sparse and indexed by
> the UID, but I guess very file name "quota tree" means this is not correct.
> Is there some document/comment that describes the on-disk quota file format?

It is really 26M different UIDs/GIDs/ProjectIDs. The sparse file format you
describe is the original quota format (implemented by fs/quota/quota_v1.c)
that the current format superseeded sometime in 1999 or so ;). The current
format uses radix tree for structure lookup and then structures are just
stored in a linear array. There's documentation of the format in
quota-tools in doc/quotadoc.sgml.

> In any case, the change makes sense regardless, since ->quota_read() takes
> loff_t for the offset.
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks!

									Honza
> 
> > ---
> > fs/quota/quota_tree.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/quota/quota_tree.c b/fs/quota/quota_tree.c
> > index a6f856f341dc..c5562c871c8b 100644
> > --- a/fs/quota/quota_tree.c
> > +++ b/fs/quota/quota_tree.c
> > @@ -62,7 +62,7 @@ static ssize_t read_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> > 
> > 	memset(buf, 0, info->dqi_usable_bs);
> > 	return sb->s_op->quota_read(sb, info->dqi_type, buf,
> > -	       info->dqi_usable_bs, blk << info->dqi_blocksize_bits);
> > +	       info->dqi_usable_bs, (loff_t)blk << info->dqi_blocksize_bits);
> > }
> > 
> > static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> > @@ -71,7 +71,7 @@ static ssize_t write_blk(struct qtree_mem_dqinfo *info, uint blk, char *buf)
> > 	ssize_t ret;
> > 
> > 	ret = sb->s_op->quota_write(sb, info->dqi_type, buf,
> > -	       info->dqi_usable_bs, blk << info->dqi_blocksize_bits);
> > +	       info->dqi_usable_bs, (loff_t)blk << info->dqi_blocksize_bits);
> > 	if (ret != info->dqi_usable_bs) {
> > 		quota_error(sb, "dquota write failed");
> > 		if (ret >= 0)
> > @@ -284,7 +284,7 @@ static uint find_free_dqentry(struct qtree_mem_dqinfo *info,
> > 			    blk);
> > 		goto out_buf;
> > 	}
> > -	dquot->dq_off = (blk << info->dqi_blocksize_bits) +
> > +	dquot->dq_off = ((loff_t)blk << info->dqi_blocksize_bits) +
> > 			sizeof(struct qt_disk_dqdbheader) +
> > 			i * info->dqi_entry_size;
> > 	kfree(buf);
> > @@ -559,7 +559,7 @@ static loff_t find_block_dqentry(struct qtree_mem_dqinfo *info,
> > 		ret = -EIO;
> > 		goto out_buf;
> > 	} else {
> > -		ret = (blk << info->dqi_blocksize_bits) + sizeof(struct
> > +		ret = ((loff_t)blk << info->dqi_blocksize_bits) + sizeof(struct
> > 		  qt_disk_dqdbheader) + i * info->dqi_entry_size;
> > 	}
> > out_buf:
> > --
> > 2.16.4
> > 
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-03  8:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 17:27 [PATCH 0/2] quota: Handle corrupted quota file better Jan Kara
2020-11-02 17:27 ` [PATCH 1/2] quota: Don't overflow quota file offsets Jan Kara
2020-11-02 21:30   ` Andreas Dilger
2020-11-03  8:57     ` Jan Kara
2020-11-02 17:27 ` [PATCH 2/2] quota: Sanity-check quota file headers on load Jan Kara
2020-11-02 21:32   ` Andreas Dilger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.