[PATCH v2] nfs: check return value of various rpc calls

[PATCH v2] nfs: check return value of various rpc calls

[Uwe Kleine-König]

Check more carefully for failing requests. This improves the error
message when trying to mount a non-exported nfs directory from:

	nfs_mount_req: file handle too big: 44831

to

	nfs_mount_req: Mounting failed: NFS3ERR_ACCES(0xd)

. This also fixes an out-of-bounds access as the filehandle size (44831
above) is read from just after the network packet in the error case.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

while working on this patch I decided to keep the error name constants
and so deviate from what I promised in reply to Sascha's feedback for
v1. The reason is that not all NFS error constants have a corresponding
POSIX error code and so pretty printing the error might be helpful.

Best regards
Uwe

 fs/nfs.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 102 insertions(+), 10 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index 15ddab7915df..1f60cbad4045 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -636,13 +636,82 @@ static uint32_t *nfs_read_post_op_attr(uint32_t *p, struct inode *inode)
 	return p;
 }
 
+static int nfserror_to_err(u32 nfserror, const char **errorname)
+{
+	/*
+	 * Most NFS errors have a corresponding POSIX error code. But not all of
+	 * them have one, so some must be mapped to a different code here.
+	 * To get helpful error messages also keep string constants for the
+	 * actual errors. They are stored without the common NFS3ERR_ prefix to
+	 * save some memory.
+	 */
+#define E(NFSERR, ERR)			\
+	case NFS3ERR_ ## NFSERR:	\
+		*errorname = #NFSERR;	\
+		return -ERR
+
+	switch (nfserror) {
+	E(PERM, EPERM);
+	E(NOENT, ENOENT);
+	E(IO, EIO);
+	E(NXIO, ENXIO);
+	E(ACCES, EACCES);
+	E(EXIST, EEXIST);
+	E(XDEV, EXDEV);
+	E(NODEV, ENODEV);
+	E(NOTDIR, ENOTDIR);
+	E(ISDIR, EISDIR);
+	E(INVAL, EINVAL);
+	E(FBIG, EFBIG);
+	E(NOSPC, ENOSPC);
+	E(ROFS, EROFS);
+	E(MLINK, EMLINK);
+	E(NAMETOOLONG, ENAMETOOLONG);
+	E(NOTEMPTY, ENOTEMPTY);
+	E(DQUOT, EDQUOT);
+	E(STALE, ESTALE);
+	E(REMOTE, EREMOTE);
+	E(BADHANDLE, EINVAL);
+	E(NOT_SYNC, EINVAL);
+	E(BAD_COOKIE, EINVAL);
+	E(NOTSUPP, ENOTSUPP);
+	E(TOOSMALL, EINVAL);
+	E(SERVERFAULT, EINVAL);
+	E(BADTYPE, EINVAL);
+	E(JUKEBOX, EINVAL);
+
+	default:
+		*errorname = "???";
+		return -EINVAL;
+	}
+}
+
+static int nfs_printerr(u32 nfserror, const char *fmt, ...)
+{
+	va_list args;
+	const char *errorname;
+	int ret;
+
+	va_start(args, fmt);
+
+	vprintf(fmt, args);
+
+	va_end(args);
+
+	ret = nfserror_to_err(nfserror, &errorname);
+
+	printf(": NFS3ERR_%s(0x%x)\n", errorname, (unsigned)nfserror);
+
+	return ret;
+}
+
 /*
  * nfs_mount_req - Mount an NFS Filesystem
  */
 static int nfs_mount_req(struct nfs_priv *npriv)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	int pathlen;
 	struct packet *nfs_packet;
@@ -667,7 +736,15 @@ static int nfs_mount_req(struct nfs_priv *npriv)
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+
+	/*
+	 * Theoretically the error status is one of MNT3ERR_..., but the NFS
+	 * constants are identical.
+	 */
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK)
+		return nfs_printerr(status, "%s: Mounting failed", __func__);
 
 	npriv->rootfh.size = ntoh32(net_read_uint32(p++));
 	if (npriv->rootfh.size > NFS3_FHSIZE) {
@@ -719,7 +796,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 {
 	struct nfs_inode *ninode = nfsi(inode);
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	struct packet *nfs_packet;
 
@@ -761,7 +838,10 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK)
+		return nfs_printerr(status, "%s: LOOKUP failed", __func__);
 
 	ninode->fh.size = ntoh32(net_read_uint32(p++));
 	if (ninode->fh.size > NFS3_FHSIZE) {
@@ -787,7 +867,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	struct packet *nfs_packet;
 	void *buf;
@@ -845,7 +925,13 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir)
 	if (IS_ERR(nfs_packet))
 		return NULL;
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK) {
+		nfs_printerr(status, "%s: READDIR failed", __func__);
+		return NULL;
+	}
+
 	p = nfs_read_post_op_attr(p, NULL);
 
 	/* update cookieverf */
@@ -879,7 +965,7 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset,
 		uint32_t readlen)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	struct packet *nfs_packet;
 	uint32_t rlen, eof;
@@ -922,7 +1008,10 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset,
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK)
+		return nfs_printerr(status, "%s: READ failed", __func__);
 
 	p = nfs_read_post_op_attr(p, NULL);
 
@@ -981,7 +1070,7 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 			    char **target)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	uint32_t len;
 	struct packet *nfs_packet;
 
@@ -1017,7 +1106,10 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK)
+		return nfs_printerr(status, "%s: READLINK failed", __func__);
 
 	p = nfs_read_post_op_attr(p, NULL);
 
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] nfs: check return value of various rpc calls

[Sascha Hauer]

Hi Uwe,

I've rewritten this thing a little bit. First of all, this doesn't need
preprocessor tricks and also with this the nfserror to error mapping
function returns the error string, so we can convert printing the
messages to pr_* or dev_* functions. Also we use the human readable
error names for the errors we have a string for.

Sascha

-------------------------------8<--------------------------------------

From 3d764946914356eca94622a2eeeb4df459026d6d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Date: Tue, 3 Nov 2020 21:09:32 +0100
Subject: [PATCH] nfs: check return value of various rpc calls
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Check more carefully for failing requests. This improves the error
message when trying to mount a non-exported nfs directory from:

	nfs_mount_req: file handle too big: 44831
to
	ERROR: NFS: Mounting failed: Permission denied

. This also fixes an out-of-bounds access as the filehandle size (44831
above) is read from just after the network packet in the error case.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/nfs.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 115 insertions(+), 11 deletions(-)

diff --git a/fs/nfs.c b/fs/nfs.c
index 6c4637281d..db159f5ab8 100644
--- a/fs/nfs.c
+++ b/fs/nfs.c
@@ -263,6 +263,76 @@ struct nfs_dir {
 	struct nfs_fh fh;
 };
 
+struct nfserror {
+	int ne;
+	int e;
+	const char *name;
+};
+
+static struct nfserror nfserrors[] = {
+	{ .ne = NFS3ERR_PERM, .e = EPERM },
+	{ .ne = NFS3ERR_NOENT, .e = ENOENT },
+	{ .ne = NFS3ERR_IO, .e = EIO },
+	{ .ne = NFS3ERR_NXIO, .e = ENXIO },
+	{ .ne = NFS3ERR_ACCES, .e = EACCES },
+	{ .ne = NFS3ERR_EXIST, .e = EEXIST },
+	{ .ne = NFS3ERR_XDEV, .e = EXDEV },
+	{ .ne = NFS3ERR_NODEV, .e = ENODEV },
+	{ .ne = NFS3ERR_NOTDIR, .e = ENOTDIR },
+	{ .ne = NFS3ERR_ISDIR, .e = EISDIR },
+	{ .ne = NFS3ERR_INVAL, .e = EINVAL },
+	{ .ne = NFS3ERR_FBIG, .e = EFBIG },
+	{ .ne = NFS3ERR_NOSPC, .e = ENOSPC },
+	{ .ne = NFS3ERR_ROFS, .e = EROFS },
+	{ .ne = NFS3ERR_MLINK, .e = EMLINK },
+	{ .ne = NFS3ERR_NAMETOOLONG, .e = ENAMETOOLONG },
+	{ .ne = NFS3ERR_NOTEMPTY, .e = ENOTEMPTY },
+	{ .ne = NFS3ERR_DQUOT, .e = EDQUOT },
+	{ .ne = NFS3ERR_STALE, .e = ESTALE },
+	{ .ne = NFS3ERR_REMOTE, .e = EREMOTE },
+	{ .ne = NFS3ERR_NOTSUPP, .e = EOPNOTSUPP },
+	{ .ne = NFS3ERR_BADHANDLE, .e = EINVAL, .name = "BADHANDLE"},
+	{ .ne = NFS3ERR_NOT_SYNC, .e = EINVAL, .name = "NOT_SYNC" },
+	{ .ne = NFS3ERR_BAD_COOKIE, .e = EINVAL, .name = "BAD_COOKIE" },
+	{ .ne = NFS3ERR_TOOSMALL, .e = EINVAL, .name = "TOOSMALL" },
+	{ .ne = NFS3ERR_SERVERFAULT, .e = EINVAL, .name = "SERVERFAULT" },
+	{ .ne = NFS3ERR_BADTYPE, .e = EINVAL, .name = "BADTYPE" },
+	{ .ne = NFS3ERR_JUKEBOX, .e = EINVAL, .name = "JUKEBOX" },
+};
+
+static const char *nfserrstr(u32 nfserror, int *errcode)
+{
+#define BUFLEN 32
+	static char str[BUFLEN];
+	int i;
+
+	/*
+	 * Most NFS errors have a corresponding POSIX error code. But not all of
+	 * them have one, so some must be mapped to a different code here.
+	 */
+	for (i = 0; i < ARRAY_SIZE(nfserrors); i++) {
+		struct nfserror *err = &nfserrors[i];
+
+		if (nfserror == err->ne) {
+			if (errcode)
+				*errcode = -err->e;
+
+			if (err->name) {
+				snprintf(str, BUFLEN, "NFS3ERR_%s", err->name);
+				return str;
+			} else
+				return strerror(err->e);
+		}
+	}
+
+	if (errcode)
+		*errcode = -EINVAL;
+
+	snprintf(str, BUFLEN, "Unknown NFS error %d", nfserror);
+	return str;
+#undef BUFLEN
+}
+
 static void xdr_init(struct xdr_stream *stream, void *buf, int len)
 {
 	stream->p = stream->buf = buf;
@@ -642,7 +712,7 @@ static uint32_t *nfs_read_post_op_attr(uint32_t *p, struct inode *inode)
 static int nfs_mount_req(struct nfs_priv *npriv)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	int pathlen;
 	struct packet *nfs_packet;
@@ -667,7 +737,18 @@ static int nfs_mount_req(struct nfs_priv *npriv)
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+
+	/*
+	 * Theoretically the error status is one of MNT3ERR_..., but the NFS
+	 * constants are identical.
+	 */
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK) {
+		int ret;
+		pr_err("Mounting failed: %s\n", nfserrstr(status, &ret));
+		return ret;
+	}
 
 	npriv->rootfh.size = ntoh32(net_read_uint32(p++));
 	if (npriv->rootfh.size > NFS3_FHSIZE) {
@@ -719,7 +800,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 {
 	struct nfs_inode *ninode = nfsi(inode);
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	struct packet *nfs_packet;
 
@@ -761,7 +842,13 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK) {
+		int ret;
+		pr_err("Lookup failed: %s\n", nfserrstr(status, &ret));
+		return ret;
+	}
 
 	ninode->fh.size = ntoh32(net_read_uint32(p++));
 	if (ninode->fh.size > NFS3_FHSIZE) {
@@ -787,7 +874,7 @@ static int nfs_lookup_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	int len;
 	struct packet *nfs_packet;
 	void *buf;
@@ -845,7 +932,13 @@ static void *nfs_readdirattr_req(struct nfs_priv *npriv, struct nfs_dir *dir)
 	if (IS_ERR(nfs_packet))
 		return NULL;
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK) {
+		pr_err("Readdir failed: %s\n", nfserrstr(status, NULL));
+		return NULL;
+	}
+
 	p = nfs_read_post_op_attr(p, NULL);
 
 	/* update cookieverf */
@@ -879,8 +972,8 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset,
 		uint32_t readlen)
 {
 	uint32_t data[1024];
-	uint32_t *p;
-	int len;
+	uint32_t *p, status;
+	int len, ret;
 	struct packet *nfs_packet;
 	uint32_t rlen, eof;
 
@@ -922,7 +1015,12 @@ static int nfs_read_req(struct file_priv *priv, uint64_t offset,
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK) {
+		pr_err("Read failed: %s\n", nfserrstr(status, &ret));
+		return ret;
+	}
 
 	p = nfs_read_post_op_attr(p, NULL);
 
@@ -981,7 +1079,7 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 			    char **target)
 {
 	uint32_t data[1024];
-	uint32_t *p;
+	uint32_t *p, status;
 	uint32_t len;
 	struct packet *nfs_packet;
 
@@ -1017,7 +1115,13 @@ static int nfs_readlink_req(struct nfs_priv *npriv, struct nfs_fh *fh,
 	if (IS_ERR(nfs_packet))
 		return PTR_ERR(nfs_packet);
 
-	p = (void *)nfs_packet->data + sizeof(struct rpc_reply) + 4;
+	p = (void *)nfs_packet->data + sizeof(struct rpc_reply);
+	status = ntoh32(net_read_uint32(p++));
+	if (status != NFS3_OK) {
+		int ret;
+		pr_err("Readlink failed: %s\n", nfserrstr(status, &ret));
+		return ret;
+	}
 
 	p = nfs_read_post_op_attr(p, NULL);
 
-- 
2.20.1

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] nfs: check return value of various rpc calls

[Gavin Schenk]

Hi,

> I've rewritten this thing a little bit. First of all, this doesn't need
> preprocessor tricks and also with this the nfserror to error mapping
> function returns the error string, so we can convert printing the
> messages to pr_* or dev_* functions. Also we use the human readable
> error names for the errors we have a string for.
>
Nice to here that there is progress.

> +
> +static const char *nfserrstr(u32 nfserror, int *errcode)
> +{

Instead using this Preprocessor thing wouldn't it be better to use a number and
sizeof(str) at the other locations?

> +     static char str[BUFLEN];
static char str[32];

here
> +                             snprintf(str, BUFLEN, "NFS3ERR_%s", err->name);
snprintf(str, sizeof(str), "NFS3ERR_%s", err->name);


and here
> +     snprintf(str, BUFLEN, "Unknown NFS error %d", nfserror);
snprintf(str, sizeof(str), "Unknown NFS error %d", nfserror);


and Remove this:
> +#define BUFLEN 32
> +#undef BUFLEN


--
Regards
Gavin Schenk
Eckelmann AG
Vorstand: Dipl.-Ing. Peter Frankenbach (Sprecher) Dipl.-Wi.-Ing. Philipp Eckelmann
Dr.-Ing. Marco Münchhof Dr.-Ing. Frank Uhlemann
Vorsitzender des Aufsichtsrats: Hubertus G. Krossa Stv. Vorsitzender des Aufsichtsrats: Dr.-Ing. Gerd Eckelmann
Sitz der Gesellschaft: Berliner Str. 161, 65205 Wiesbaden, Amtsgericht Wiesbaden HRB 12636
www.eckelmann.de

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] nfs: check return value of various rpc calls

[Sascha Hauer]

On Mon, Nov 09, 2020 at 03:37:23PM +0100, Gavin Schenk wrote:
> Hi,
> 
> > I've rewritten this thing a little bit. First of all, this doesn't need
> > preprocessor tricks and also with this the nfserror to error mapping
> > function returns the error string, so we can convert printing the
> > messages to pr_* or dev_* functions. Also we use the human readable
> > error names for the errors we have a string for.
> >
> Nice to here that there is progress.
> 
> > +
> > +static const char *nfserrstr(u32 nfserror, int *errcode)
> > +{
> 
> Instead using this Preprocessor thing wouldn't it be better to use a number and
> sizeof(str) at the other locations?
> 
> > +	static char str[BUFLEN];
> static char str[32];
> 
> here
> > +				snprintf(str, BUFLEN, "NFS3ERR_%s", err->name);
> snprintf(str, sizeof(str), "NFS3ERR_%s", err->name);
> 
> 
> and here
> > +	snprintf(str, BUFLEN, "Unknown NFS error %d", nfserror);
> snprintf(str, sizeof(str), "Unknown NFS error %d", nfserror);

Much better, thanks. I'll change that

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox