[PATCH v2] fs: make d_path-like functions all have unsigned size

[PATCH v2] fs: make d_path-like functions all have unsigned size

[Greg Kroah-Hartman]

When running static analysis tools to find where signed values could
potentially wrap the family of d_path() functions turn out to trigger a
lot of mess.  In evaluating the code, all of these usages seem safe, but
pointer math is involved so if a negative number is ever somehow passed
into these functions, memory can be traversed backwards in ways not
intended.

Resolve all of the abuguity by just making "size" an unsigned value,
which takes the guesswork out of everything involved.

Reported-by: Jordy Zomer <jordy@pwning.systems>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "Ahmed S. Darwish" <a.darwish@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Eric Biggers <ebiggers@google.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
changes since v1:
	- add 'size' name to function prototypes
	- change struct prepend_buffer's size field to also be unsigned

 fs/d_path.c            | 16 ++++++++--------
 include/linux/dcache.h | 16 ++++++++--------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/d_path.c b/fs/d_path.c
index 23a53f7b5c71..73b7ea17a330 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -10,7 +10,7 @@
 
 struct prepend_buffer {
 	char *buf;
-	int len;
+	unsigned int len;
 };
 #define DECLARE_BUFFER(__name, __buf, __len) \
 	struct prepend_buffer __name = {.buf = __buf + __len, .len = __len}
@@ -182,7 +182,7 @@ static int prepend_path(const struct path *path,
  */
 char *__d_path(const struct path *path,
 	       const struct path *root,
-	       char *buf, int buflen)
+	       char *buf, unsigned int buflen)
 {
 	DECLARE_BUFFER(b, buf, buflen);
 
@@ -193,7 +193,7 @@ char *__d_path(const struct path *path,
 }
 
 char *d_absolute_path(const struct path *path,
-	       char *buf, int buflen)
+	       char *buf, unsigned int buflen)
 {
 	struct path root = {};
 	DECLARE_BUFFER(b, buf, buflen);
@@ -230,7 +230,7 @@ static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
  *
  * "buflen" should be positive.
  */
-char *d_path(const struct path *path, char *buf, int buflen)
+char *d_path(const struct path *path, char *buf, unsigned int buflen)
 {
 	DECLARE_BUFFER(b, buf, buflen);
 	struct path root;
@@ -266,7 +266,7 @@ EXPORT_SYMBOL(d_path);
 /*
  * Helper function for dentry_operations.d_dname() members
  */
-char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
+char *dynamic_dname(struct dentry *dentry, char *buffer, unsigned int buflen,
 			const char *fmt, ...)
 {
 	va_list args;
@@ -284,7 +284,7 @@ char *dynamic_dname(struct dentry *dentry, char *buffer, int buflen,
 	return memcpy(buffer, temp, sz);
 }
 
-char *simple_dname(struct dentry *dentry, char *buffer, int buflen)
+char *simple_dname(struct dentry *dentry, char *buffer, unsigned int buflen)
 {
 	DECLARE_BUFFER(b, buffer, buflen);
 	/* these dentries are never renamed, so d_lock is not needed */
@@ -328,7 +328,7 @@ static char *__dentry_path(const struct dentry *d, struct prepend_buffer *p)
 	return extract_string(&b);
 }
 
-char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
+char *dentry_path_raw(const struct dentry *dentry, char *buf, unsigned int buflen)
 {
 	DECLARE_BUFFER(b, buf, buflen);
 
@@ -337,7 +337,7 @@ char *dentry_path_raw(const struct dentry *dentry, char *buf, int buflen)
 }
 EXPORT_SYMBOL(dentry_path_raw);
 
-char *dentry_path(const struct dentry *dentry, char *buf, int buflen)
+char *dentry_path(const struct dentry *dentry, char *buf, unsigned int buflen)
 {
 	DECLARE_BUFFER(b, buf, buflen);
 
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 9e23d33bb6f1..c93ac4e39566 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -295,14 +295,14 @@ static inline unsigned d_count(const struct dentry *dentry)
 /*
  * helper function for dentry_operations.d_dname() members
  */
-extern __printf(4, 5)
-char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
-
-extern char *__d_path(const struct path *, const struct path *, char *, int);
-extern char *d_absolute_path(const struct path *, char *, int);
-extern char *d_path(const struct path *, char *, int);
-extern char *dentry_path_raw(const struct dentry *, char *, int);
-extern char *dentry_path(const struct dentry *, char *, int);
+__printf(4, 5)
+char *dynamic_dname(struct dentry *, char *, unsigned int size , const char *, ...);
+
+char *__d_path(const struct path *, const struct path *, char *, unsigned int size);
+char *d_absolute_path(const struct path *, char *, unsigned int size);
+char *d_path(const struct path *, char *, unsigned int size);
+char *dentry_path_raw(const struct dentry *, char *, unsigned int size);
+char *dentry_path(const struct dentry *, char *, unsigned int size);
 
 /* Allocation counts.. */
 
-- 
2.32.0

Re: [PATCH v2] fs: make d_path-like functions all have unsigned size

[Andy Shevchenko]

On Tue, Jul 27, 2021 at 02:07:54PM +0200, Greg Kroah-Hartman wrote:
> When running static analysis tools to find where signed values could
> potentially wrap the family of d_path() functions turn out to trigger a
> lot of mess.  In evaluating the code, all of these usages seem safe, but
> pointer math is involved so if a negative number is ever somehow passed
> into these functions, memory can be traversed backwards in ways not
> intended.
> 
> Resolve all of the abuguity by just making "size" an unsigned value,
> which takes the guesswork out of everything involved.

Are you sure it's correct change?

Look into extract_string() implementation.

	if (likely(p->len >= 0))
		return p->buf;
	return ERR_PTR(-ENAMETOOLONG);

Your change makes it equal to

	return p->buf;

if I'm not mistaken.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] fs: make d_path-like functions all have unsigned size

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 03:39:31PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 27, 2021 at 02:07:54PM +0200, Greg Kroah-Hartman wrote:
> > When running static analysis tools to find where signed values could
> > potentially wrap the family of d_path() functions turn out to trigger a
> > lot of mess.  In evaluating the code, all of these usages seem safe, but
> > pointer math is involved so if a negative number is ever somehow passed
> > into these functions, memory can be traversed backwards in ways not
> > intended.
> > 
> > Resolve all of the abuguity by just making "size" an unsigned value,
> > which takes the guesswork out of everything involved.
> 
> Are you sure it's correct change?
> 
> Look into extract_string() implementation.
> 
> 	if (likely(p->len >= 0))
> 		return p->buf;
> 	return ERR_PTR(-ENAMETOOLONG);
> 
> Your change makes it equal to
> 
> 	return p->buf;
> 
> if I'm not mistaken.

Yes it does, you are right.  So now we don't need to check the wrap
there :)

So this code is explicitly wanting the value to wrap into a negative
value to check for problems, didn't expect that.

Still feels very fragile, if you look at the documentation for __d_path,
it says:
	"buflen" should be positive.
and if you look at who calls it, they are all passing in an unsigned
value, seq_path_root() uses a size_t as buflen.  What's the issues
involved there when size_t is a unsigned value going into a signed int?

And my mistake from earlier, size_t is the same as unsigned int, not
unsigned long.

Anyway, this code feels subtle and tricky here, such that parsing tools
warn "hey, something might be wrong here, check it out!"

I'm not set on changing prepend_buffer->len, but I will not complain if
it is, but we might want to have a different check in extract_string()
and prepend() to verify that p->len does not go bigger than
MAX_SOMETHING?

But in the end, you are right, this version of the patch is not ok, all
of the checks for len being < 0 are now moot, gotta love the fact that
gcc didn't say squat about that :(

thanks,

greg k-h

Re: [PATCH v2] fs: make d_path-like functions all have unsigned size

[Matthew Wilcox]

On Tue, Jul 27, 2021 at 02:56:53PM +0200, Greg Kroah-Hartman wrote:
> And my mistake from earlier, size_t is the same as unsigned int, not
> unsigned long.

No.

include/linux/types.h:typedef __kernel_size_t           size_t;

include/uapi/asm-generic/posix_types.h:

#ifndef __kernel_size_t
#if __BITS_PER_LONG != 64
typedef unsigned int    __kernel_size_t;
#else
typedef __kernel_ulong_t __kernel_size_t;
#endif
#endif

size_t is an unsigned long on 64-bit, unless otherwise defined by the
arch.

Re: [PATCH v2] fs: make d_path-like functions all have unsigned size

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 02:14:37PM +0100, Matthew Wilcox wrote:
> On Tue, Jul 27, 2021 at 02:56:53PM +0200, Greg Kroah-Hartman wrote:
> > And my mistake from earlier, size_t is the same as unsigned int, not
> > unsigned long.
> 
> No.
> 
> include/linux/types.h:typedef __kernel_size_t           size_t;
> 
> include/uapi/asm-generic/posix_types.h:
> 
> #ifndef __kernel_size_t
> #if __BITS_PER_LONG != 64
> typedef unsigned int    __kernel_size_t;
> #else
> typedef __kernel_ulong_t __kernel_size_t;
> #endif
> #endif
> 
> size_t is an unsigned long on 64-bit, unless otherwise defined by the
> arch.

ugh, ok, so there really is a problem, as we have a size_t value being
passed in as an int, and then it could be treated as a negative value
for some fun pointer math to copy buffers around.

How is this not causing problems now already?  Are we just getting
lucky?

thanks,

greg k-h

Re: [PATCH v2] fs: make d_path-like functions all have unsigned size

[Matthew Wilcox]

On Tue, Jul 27, 2021 at 03:30:55PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 27, 2021 at 02:14:37PM +0100, Matthew Wilcox wrote:
> > On Tue, Jul 27, 2021 at 02:56:53PM +0200, Greg Kroah-Hartman wrote:
> > > And my mistake from earlier, size_t is the same as unsigned int, not
> > > unsigned long.
> > 
> > No.
> > 
> > include/linux/types.h:typedef __kernel_size_t           size_t;
> > 
> > include/uapi/asm-generic/posix_types.h:
> > 
> > #ifndef __kernel_size_t
> > #if __BITS_PER_LONG != 64
> > typedef unsigned int    __kernel_size_t;
> > #else
> > typedef __kernel_ulong_t __kernel_size_t;
> > #endif
> > #endif
> > 
> > size_t is an unsigned long on 64-bit, unless otherwise defined by the
> > arch.
> 
> ugh, ok, so there really is a problem, as we have a size_t value being
> passed in as an int, and then it could be treated as a negative value
> for some fun pointer math to copy buffers around.
> 
> How is this not causing problems now already?  Are we just getting
> lucky?

include/uapi/linux/limits.h:#define PATH_MAX        4096        /* # chars in a path name including nul */

Clearly some places aren't checking that, but _in principle_, you
aren't supposed to be able to create a pathname longer than that.