[PATCH] ovl: Fix a potential memory leak for kstrdup()

[PATCH] ovl: Fix a potential memory leak for kstrdup()

[williamsukatube]

From: William Dean <williamsukatube@gmail.com>

kfree() is missing on an error path to free the memory
allocated by kstrdup():

config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);

So it is better to free it via kfree(config->redirect_mode).

Reported-by: Hacash Robot <hacashRobot@santino.com>
Signed-off-by: William Dean <williamsukatube@gmail.com>
---
 fs/overlayfs/super.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index b936e2c9226b..6e95ea078915 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -533,22 +533,28 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		case OPT_UPPERDIR:
 			kfree(config->upperdir);
 			config->upperdir = match_strdup(&args[0]);
-			if (!config->upperdir)
-				return -ENOMEM;
+			if (!config->upperdir) {
+				err = -ENOMEM;
+				goto out_err;
+			}
 			break;
 
 		case OPT_LOWERDIR:
 			kfree(config->lowerdir);
 			config->lowerdir = match_strdup(&args[0]);
-			if (!config->lowerdir)
-				return -ENOMEM;
+			if (!config->lowerdir) {
+				err = -ENOMEM;
+				goto out_err;
+			}
 			break;
 
 		case OPT_WORKDIR:
 			kfree(config->workdir);
 			config->workdir = match_strdup(&args[0]);
-			if (!config->workdir)
-				return -ENOMEM;
+			if (!config->workdir) {
+				err = -ENOMEM;
+				goto out_err;
+			}
 			break;
 
 		case OPT_DEFAULT_PERMISSIONS:
@@ -624,7 +630,8 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		default:
 			pr_err("unrecognized mount option \"%s\" or missing value\n",
 					p);
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_err;
 		}
 	}
 
@@ -650,7 +657,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 
 	err = ovl_parse_redirect_mode(config, config->redirect_mode);
 	if (err)
-		return err;
+		goto out_err;
 
 	/*
 	 * This is to make the logic below simpler.  It doesn't make any other
@@ -664,7 +671,8 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		if (metacopy_opt && redirect_opt) {
 			pr_err("conflicting options: metacopy=on,redirect_dir=%s\n",
 			       config->redirect_mode);
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_err;
 		}
 		if (redirect_opt) {
 			/*
@@ -687,7 +695,8 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			config->nfs_export = false;
 		} else if (nfs_export_opt && index_opt) {
 			pr_err("conflicting options: nfs_export=on,index=off\n");
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_err;
 		} else if (index_opt) {
 			/*
 			 * There was an explicit index=off that resulted
@@ -705,7 +714,8 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	if (config->nfs_export && config->metacopy) {
 		if (nfs_export_opt && metacopy_opt) {
 			pr_err("conflicting options: nfs_export=on,metacopy=on\n");
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_err;
 		}
 		if (metacopy_opt) {
 			/*
@@ -730,11 +740,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		if (config->redirect_follow && redirect_opt) {
 			pr_err("conflicting options: userxattr,redirect_dir=%s\n",
 			       config->redirect_mode);
-			return -EINVAL;
+			err =  -EINVAL;
+			goto out_err;
 		}
 		if (config->metacopy && metacopy_opt) {
 			pr_err("conflicting options: userxattr,metacopy=on\n");
-			return -EINVAL;
+			err = -EINVAL;
+			goto out_err;
 		}
 		/*
 		 * Silently disable default setting of redirect and metacopy.
@@ -747,6 +759,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 	}
 
 	return 0;
+
+out_err:
+	kfree(config->redirect_mode);
+	return err;
 }
 
 #define OVL_WORKDIR_NAME "work"
-- 
2.25.1

Re: [PATCH] ovl: Fix a potential memory leak for kstrdup()

[Miklos Szeredi]

On Wed, 13 Jul 2022 at 09:33, <williamsukatube@163.com> wrote:
>
> From: William Dean <williamsukatube@gmail.com>
>
> kfree() is missing on an error path to free the memory
> allocated by kstrdup():
>
> config->redirect_mode = kstrdup(ovl_redirect_mode_def(), GFP_KERNEL);
>
> So it is better to free it via kfree(config->redirect_mode).

Will be freed in ovl_free_fs().

Thanks,
Miklos