[RFC PATCH] quota: Fix possible GFP due to uninitialised pointers

[RFC PATCH] quota: Fix possible GFP due to uninitialised pointers

[Nikolay Borisov]

While debugging some issues with quota I realized that
it's possible to pass array with bogus dquot pointers from
__dquot_initialize to dqput. This can happen if the initialisation
of the dquot objects for an inode fail and the control flow is
transferred to the out_put label. In case only the USR or GRP quota
are initialised then the PRJ pointer in the "got" array would remain
uninitialised. This will cause the NULL ptr check in dqput to pass
but actually the pointer is going to be invalid. Eventually this would
cause a GFP.

To fix this just zero out the got array

Signed-off-by: Nikolay Borisov <kernel@kyup.com>
---
 fs/quota/dquot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index ef0d64b2a6d9..a0ab58fd85ae 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1408,6 +1408,8 @@ static int __dquot_initialize(struct inode *inode, int type)
 
 	dquots = i_dquot(inode);
 
+	memset(got, 0, 3 * sizeof(struct dquot *));
+
 	/* First get references to structures we might need. */
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		struct kqid qid;
-- 
2.5.0

Re: [RFC PATCH] quota: Fix possible GFP due to uninitialised pointers

[Jan Kara]

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

On Wed 02-03-16 18:19:29, Nikolay Borisov wrote:
> While debugging some issues with quota I realized that
> it's possible to pass array with bogus dquot pointers from
> __dquot_initialize to dqput. This can happen if the initialisation
> of the dquot objects for an inode fail and the control flow is
> transferred to the out_put label. In case only the USR or GRP quota
> are initialised then the PRJ pointer in the "got" array would remain
> uninitialised. This will cause the NULL ptr check in dqput to pass
> but actually the pointer is going to be invalid. Eventually this would
> cause a GFP.
> 
> To fix this just zero out the got array
> 
> Signed-off-by: Nikolay Borisov <kernel@kyup.com>

Thanks for spotting this and for the fix. There are couple of issues with
your patch:

a) You should use MAXQUOTAS instead of hardcoded 3 in the memset(). Even
better just leave, specify proper initializer directly.

b) You could remove the array initialization from the for loop.

I have cleaned up the patch as attached and merged it into my tree.

								Honza

> ---
>  fs/quota/dquot.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index ef0d64b2a6d9..a0ab58fd85ae 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1408,6 +1408,8 @@ static int __dquot_initialize(struct inode *inode, int type)
>  
>  	dquots = i_dquot(inode);
>  
> +	memset(got, 0, 3 * sizeof(struct dquot *));
> +
>  	/* First get references to structures we might need. */
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>  		struct kqid qid;
> -- 
> 2.5.0
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-quota-Fix-possible-GPF-due-to-uninitialised-pointers.patch --]
[-- Type: text/x-patch, Size: 1299 bytes --]

>From 130248216a62ab08b628d7c35b33842fb213fc7b Mon Sep 17 00:00:00 2001
From: Nikolay Borisov <kernel@kyup.com>
Date: Thu, 3 Mar 2016 10:54:57 +0100
Subject: [PATCH] quota: Fix possible GPF due to uninitialised pointers

When dqget() in __dquot_initialize() fails e.g. due to IO error,
__dquot_initialize() will pass an array of uninitialized pointers to
dqput_all() and thus can lead to deference of random data. Fix the
problem by properly initializing the array.

Signed-off-by: Nikolay Borisov <kernel@kyup.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index e8467dc90c7e..dcec1edf579f 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -1410,7 +1410,7 @@ static int dquot_active(const struct inode *inode)
 static int __dquot_initialize(struct inode *inode, int type)
 {
 	int cnt, init_needed = 0;
-	struct dquot **dquots, *got[MAXQUOTAS];
+	struct dquot **dquots, *got[MAXQUOTAS] = {};
 	struct super_block *sb = inode->i_sb;
 	qsize_t rsv;
 	int ret = 0;
@@ -1427,7 +1427,6 @@ static int __dquot_initialize(struct inode *inode, int type)
 		int rc;
 		struct dquot *dquot;
 
-		got[cnt] = NULL;
 		if (type != -1 && cnt != type)
 			continue;
 		/*
-- 
2.6.2