Skip to contentSkip to conversation

nfs: don't pass non-NULL-terminated string to pr_notice()

PATCH In review Jeff Layton 2 participants 4 messages Apr 4, 2014 – Apr 5, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Jeff Layton

author
#0 lore raw

PATCH 1/1 nfs: don't pass non-NULL-terminated string to pr_notice()

There is no guarantee that the strings in the nfs_cache_array will be NULL-terminated. In the event that we end up hitting a readdir loop, we need to ensure that we pass the warning message a properly-terminated string.

Reported-by Lachlan McIlroy <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/dir.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
fs/nfs/dir.c +4 -3
index 5c0b6ecc3a88..4689b125f9fe 100644
@@ -304,12 +304,13 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 				if (ctx->duped > 0
 				    && ctx->dup_cookie == *desc->dir_cookie) {
 					if (printk_ratelimit()) {
+						char *name = kstrndup(array->array[i].string.name, array->array[i].string.len, GFP_KERNEL);
+
 						pr_notice("NFS: directory %pD2 contains a readdir loop."
 								"Please contact your server vendor.  "
 								"The file: %s has duplicate cookie %llu\n",
-								desc->file,
-								array->array[i].string.name,
-								*desc->dir_cookie);
+								desc->file, name, *desc->dir_cookie);
+						kfree(name);
 					}
 					status = -ELOOP;
 					goto out;
Signature · 1 line
1.9.0
#1 lore raw
Quoted 20 lines · Jeff Layton

There is no guarantee that the strings in the nfs_cache_array will be NULL-terminated. In the event that we end up hitting a readdir loop, we need to ensure that we pass the warning message a properly-terminated string.

Reported-by Lachlan McIlroy <[email protected]>
Signed-off-by Jeff Layton <[email protected]>

---
fs/nfs/dir.c | 7 ++++---

1 file changed, 4 insertions(+), 3 deletions(-)
fs/nfs/dir.c +1 -0
index 5c0b6ecc3a88..4689b125f9fe 100644
@@ -304,12 +304,13 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 				if (ctx->duped > 0
 				    && ctx->dup_cookie == *desc->dir_cookie) {
 					if (printk_ratelimit()) {
+						char *name = kstrndup(array->array[i].string.name, array->array[i].string.len, GFP_KERNEL);

fs/nfs/dir.c int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des · line 314

Quoted 10 lines
fs/nfs/dir.c +3 -3
+
 						pr_notice("NFS: directory %pD2 contains a readdir loop."
 								"Please contact your server vendor.  "
 								"The file: %s has duplicate cookie %llu\n",
-								desc->file,
-								array->array[i].string.name,
-								*desc->dir_cookie);
+								desc->file, name, *desc->dir_cookie);
+						kfree(name);
 					}

Umm… Any reason why we couldn’t just use ‘%.*s’ ?

_________________________________
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

author
#2 lore raw
Quoted 27 lines · On Fri, 4 Apr 2014 09:51:11 -0400 Trond Myklebust…
Quoted 24 lines · Jeff Layton

There is no guarantee that the strings in the nfs_cache_array will be NULL-terminated. In the event that we end up hitting a readdir loop, we need to ensure that we pass the warning message a properly-terminated string.

Reported-by Lachlan McIlroy <[email protected]>
Signed-off-by Jeff Layton <[email protected]>

---
fs/nfs/dir.c | 7 ++++---

1 file changed, 4 insertions(+), 3 deletions(-)
fs/nfs/dir.c +2 -0
index 5c0b6ecc3a88..4689b125f9fe 100644
@@ -304,12 +304,13 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 				if (ctx->duped > 0
 				    && ctx->dup_cookie == *desc->dir_cookie) {
 					if (printk_ratelimit()) {
+						char *name = kstrndup(array->array[i].string.name, array->array[i].string.len, GFP_KERNEL);
+
 						pr_notice("NFS: directory %pD2 contains a readdir loop."
 								"Please contact your server vendor.  "
 								"The file: %s has duplicate cookie %llu\n",
Quoted 9 lines

fs/nfs/dir.c int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des · line 314

Quoted 6 lines
fs/nfs/dir.c +2 -3
-								desc->file,
-								array->array[i].string.name,
-								*desc->dir_cookie);
+								desc->file, name, *desc->dir_cookie);
+						kfree(name);
 					}

Umm… Any reason why we couldn’t just use ‘%.*s’ ?

No reason at all. I had never run across that...

I'll respin and repost -- thanks!

Signature · 1 line
Jeff Layton <[email protected]>
author
#3 lore raw

PATCH v2 1/1 nfs: pass string length to pr_notice message about readdir loops

In reply to Jeff Layton

There is no guarantee that the strings in the nfs_cache_array will be NULL-terminated. In the event that we end up hitting a readdir loop, we need to ensure that we pass the warning message the length of the string.

Reported-by Lachlan McIlroy <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfs/dir.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
fs/nfs/dir.c +3 -4
index e6f8e2c97653..0013d5fc1aed 100644
@@ -306,10 +306,9 @@ int nfs_readdir_search_for_cookie(struct nfs_cache_array *array, nfs_readdir_des
 					if (printk_ratelimit()) {
 						pr_notice("NFS: directory %pD2 contains a readdir loop."
 								"Please contact your server vendor.  "
-								"The file: %s has duplicate cookie %llu\n",
-								desc->file,
-								array->array[i].string.name,
-								*desc->dir_cookie);
+								"The file: %.*s has duplicate cookie %llu\n",
+								desc->file, array->array[i].string.len,
+								array->array[i].string.name, *desc->dir_cookie);
 					}
 					status = -ELOOP;
 					goto out;
Signature · 1 line
1.9.0