linux-kernel - Re: [PATCH] Fix NFS option parsing bit-rot.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 8 Jun 2016 14:41:25 -0500
From:	Rob Landley <rob@...dley.net>
To:	Trond Myklebust <trondmy@...marydata.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Anna Schumaker <anna.schumaker@...app.com>,
	"linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>
Subject: Re: [PATCH] Fix NFS option parsing bit-rot.

On 06/06/2016 06:49 PM, Trond Myklebust wrote:
> On 6/6/16, 18:58, "Rob Landley" <rob@...dley.net> wrote:
> 
>>From: Rob Landley <rob@...dley.net>
>>
>>The kernel has string parsing code for NFS mount options, but it seems
>>to have bit-rotted over the years, so toybox mount needs the following
>>patch to be able to mount nfs. Without it, the kernel returns "invalid
>>argument" before sending any network traffic.
>>
>>For more information, see
>>http://lists.landley.net/pipermail/toybox-landley.net/2016-March/004790.html
...
> That will scribble over the parsed address.

You mean if you supply both -o addr=host and host:/ path name?
Because you can't leave off host:/ or it errors. If you do:

  mount("/tmp","/mnt","nfs",1,
    "port=9999,mountport=9999,nolock,v3,udp,addr=10.0.2.2");

It barfs because nfs_parse_devname() does:

  end = strchr(dev_name, ':');
  if (end == NULL)
    goto out_bad_devname;
...
out_bad_devname:
  dfprintk(MOUNT, "NFS: device name not in host:path format\n");
  return -EINVAL;

So addr= is at _best_ redundant. You MUST supply host:/ always,
the current code just wasn't using it. My patch makes it use it.

There's no other way to get the address set for the text
parsing path, when we enter nfs_validate_text_mount_data()
(the function I patched), mount_info.parsed will always be
zero.

But sure, he's a gratuitous for loop checking that you haven't
redundantly supplied both -o addr=host and host://path

Signed-off-by: Rob Landley <rob@...dley.net>

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 2137e02..9b62d8b 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2130,11 +2130,28 @@ static int nfs_validate_text_mount_data(void *options,
 	int port = 0;
 	int max_namelen = PAGE_SIZE;
 	int max_pathlen = NFS_MAXPATHLEN;
+	int rc;
+	int i;
+	char *c;
 	struct sockaddr *sap = (struct sockaddr *)&args->nfs_server.address;
 
 	if (nfs_parse_mount_options((char *)options, args) == 0)
 		return -EINVAL;
 
+	rc = nfs_parse_devname(dev_name,
+				   &args->nfs_server.hostname,
+				   max_namelen,
+				   &args->nfs_server.export_path,
+				   max_pathlen);
+
+	for (i = 0, c = (void *)sap; i<sizeof(*sap); i++)
+		if (c[i]) break;
+	if (i == sizeof(*sap))
+		args->nfs_server.addrlen = rpc_pton(args->net,
+				args->nfs_server.hostname,
+				strlen(args->nfs_server.hostname),
+				sap, sizeof(args->nfs_server.address));
+
 	if (!nfs_verify_server_address(sap))
 		goto out_no_address;
 
@@ -2155,11 +2172,7 @@ static int nfs_validate_text_mount_data(void *options,
 
 	nfs_set_port(sap, &args->nfs_server.port, port);
 
-	return nfs_parse_devname(dev_name,
-				   &args->nfs_server.hostname,
-				   max_namelen,
-				   &args->nfs_server.export_path,
-				   max_pathlen);
+	return rc;
 
 #if !IS_ENABLED(CONFIG_NFS_V4)
 out_v4_not_compiled: