9p coverity fixes

From: Dominique Martinet <[email protected]>

Since we already had one coverity fix for 9p, I figured I could request an account and look at stuff that actually could matter.

The leak of glock.client_id wasn't found by coverity but when I was looking at a false positive there, of the rest the rdma one is useless but the other two are pretty important -- I will probably mark the three useful ones to backport to stable kernels.

As usual, comments more than welcome, but I'll probably push them to linux-next along with the other patches that need test after testing the whole batch together next week.

Dominique Martinet (4):
9p: acl: fix uninitialized iattr access 9p/rdma: remove useless check in cm_event_handler 9p: p9dirent_read: check network-provided name length 9p locks: fix glock.client_id leak in do_lock

 fs/9p/acl.c         |  2 +-
 fs/9p/vfs_file.c    | 16 ++++++++++++++--
 net/9p/protocol.c   | 12 +++++++++---
 net/9p/trans_rdma.c |  3 +--
 4 files changed, 25 insertions(+), 8 deletions(-)

2.17.1

From: Dominique Martinet <[email protected]>

iattr is passed to v9fs_vfs_setattr_dotl which does send various values from iattr over the wire, even if it tells the server to only look at iattr.ia_valid fields this could leak some stack data.

Addresses-Coverity-ID: 1195601 ("Uninitalized scalar variable")

---
 fs/9p/acl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 082d227fa56b..6261719f6f2a 100644
@@ -276,7 +276,7 @@ static int v9fs_xattr_set_acl(const struct xattr_handler *handler,
 	switch (handler->flags) {
 	case ACL_TYPE_ACCESS:
 		if (acl) {
-			struct iattr iattr;
+			struct iattr iattr = { 0 };
 			struct posix_acl *old_acl = acl;
 
 			retval = posix_acl_update_mode(inode, &iattr.ia_mode, &acl);

2.17.1

From: Dominique Martinet <[email protected]>

the client c is always dereferenced to get the rdma struct, so c has to be a valid pointer at this point.
Gcc would optimize that away but let's make coverity happy...

Addresses-Coverity-ID: 102778 ("Dereference before null check")

---
 net/9p/trans_rdma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 9719bc4d9424..119103bfa82e 100644
@@ -274,8 +274,7 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 	case RDMA_CM_EVENT_DISCONNECTED:
 		if (rdma)
 			rdma->state = P9_RDMA_CLOSED;
-		if (c)
-			c->status = Disconnected;
+		c->status = Disconnected;
 		break;
 
 	case RDMA_CM_EVENT_TIMEWAIT_EXIT:

2.17.1

From: Dominique Martinet <[email protected]>

strcpy to dirent->d_name could overflow the buffer, use strscpy to check the provided string length and error out if the size was too big.

While we are here, make the function return an error when the pdu parsing failed, instead of returning the pdu offset as if it had been a success...

Addresses-Coverity-ID: 139133 ("Copy into fixed size buffer")

---
 net/9p/protocol.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index b4d80c533f89..462ba144cb39 100644
@@ -623,13 +623,19 @@ int p9dirent_read(struct p9_client *clnt, char *buf, int len,
 	if (ret) {
 		p9_debug(P9_DEBUG_9P, "<<< p9dirent_read failed: %d\n", ret);
 		trace_9p_protocol_dump(clnt, &fake_pdu);
-		goto out;
+		return ret;
 	}
 
-	strcpy(dirent->d_name, nameptr);
+	ret = strscpy(dirent->d_name, nameptr, sizeof(dirent->d_name));
+	if (ret < 0) {
+		p9_debug(P9_DEBUG_ERROR,
+			 "On the wire dirent name too long: %s\n",
+			 nameptr);
+		kfree(nameptr);
+		return ret;
+	}
 	kfree(nameptr);
 
-out:
 	return fake_pdu.offset;
 }
 EXPORT_SYMBOL(p9dirent_read);

2.17.1

From: Dominique Martinet <[email protected]>

the 9p client code overwrites our glock.client_id pointing to a static buffer by an allocated string holding the network provided value which we do not care about; free and reset the value as appropriate.

This is almost identical to the leak in v9fs_file_getlock() fixed by Al Viro in commit ce85dd58ad5a6 ("9p: we are leaking glock.client_id in v9fs_file_getlock()"), which was returned as an error by a coverity false positive -- while we are here attempt to make the code slightly more robust to future change of the net/9p/client code and hopefully more clear to coverity that there is no problem.

---
 fs/9p/vfs_file.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

index 73857ebaedfb..a25efa782fcc 100644
@@ -208,6 +208,14 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
 		if (schedule_timeout_interruptible(v9ses->session_lock_timeout)
 				!= 0)
 			break;
+		/*
+		 * p9_client_lock_dotl overwrites flock.client_id with the
+		 * server message, free and reuse the client name
+		 */
+		if (flock.client_id != fid->clnt->name) {
+			kfree(flock.client_id);
+			flock.client_id = fid->clnt->name;
+		}
 	}
 
 	/* map 9p status to VFS status */

@@ -239,6 +247,8 @@ static int v9fs_file_do_lock(struct file *filp, int cmd, struct file_lock *fl)
 		locks_lock_file_wait(filp, fl);
 		fl->fl_type = fl_type;
 	}
+	if (flock.client_id != fid->clnt->name)
+		kfree(flock.client_id);
 out:
 	return res;
 }

@@ -273,7 +283,7 @@ static int v9fs_file_getlock(struct file *filp, struct file_lock *fl)
 
 	res = p9_client_getlock_dotl(fid, &glock);
 	if (res < 0)
-		return res;
+		goto out;
 	/* map 9p lock type to os lock type */
 	switch (glock.type) {
 	case P9_LOCK_TYPE_RDLCK:

@@ -294,7 +304,9 @@ static int v9fs_file_getlock(struct file *filp, struct file_lock *fl)
 			fl->fl_end = glock.start + glock.length - 1;
 		fl->fl_pid = -glock.proc_id;
 	}
-	kfree(glock.client_id);
+out:
+	if (glock.client_id != fid->clnt->name)
+		kfree(glock.client_id);
 	return res;
 }
 

2.17.1