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

[PATCH] 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>
---
 fs/d_path.c            | 14 +++++++-------
 include/linux/dcache.h | 12 ++++++------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/d_path.c b/fs/d_path.c
index 23a53f7b5c71..7876b741a47e 100644
--- a/fs/d_path.c
+++ b/fs/d_path.c
@@ -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..1a9838dc66fe 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -296,13 +296,13 @@ 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 *, ...);
+char *dynamic_dname(struct dentry *, char *, unsigned 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);
+char *__d_path(const struct path *, const struct path *, char *, unsigned int);
+char *d_absolute_path(const struct path *, char *, unsigned int);
+char *d_path(const struct path *, char *, unsigned int);
+char *dentry_path_raw(const struct dentry *, char *, unsigned int);
+char *dentry_path(const struct dentry *, char *, unsigned int);
 
 /* Allocation counts.. */
 
-- 
2.32.0

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

[Ahmed S. Darwish]

On Tue, Jul 27, 2021, Greg Kroah-Hartman wrote:
>
> Resolve all of the abuguity by just making "size" an unsigned value,
> which takes the guesswork out of everything involved.
>

Pardon my ignorance, but why not size_t instead of an unsigned int? I
feel it will be more clear this way; but, yes, on 64-bit machines this
will extend the buflen param to 64-bit.

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH

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

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 12:49:52PM +0200, Ahmed S. Darwish wrote:
> On Tue, Jul 27, 2021, Greg Kroah-Hartman wrote:
> >
> > Resolve all of the abuguity by just making "size" an unsigned value,
> > which takes the guesswork out of everything involved.
> >
> 
> Pardon my ignorance, but why not size_t instead of an unsigned int? I
> feel it will be more clear this way; but, yes, on 64-bit machines this
> will extend the buflen param to 64-bit.

I have no objection moving it to size_t, but as you say, I don't think
it's needed to make the buffer get that big.

thanks,

greg k-h

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

[Matthew Wilcox]

On Tue, Jul 27, 2021 at 12:36:25PM +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.

> diff --git a/fs/d_path.c b/fs/d_path.c
> index 23a53f7b5c71..7876b741a47e 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -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);

I have questions about the quality of the analysis tool you're using.

struct prepend_buffer {
        char *buf;
        int len;
};
#define DECLARE_BUFFER(__name, __buf, __len) \
        struct prepend_buffer __name = {.buf = __buf + __len, .len = __len}

Why is it not flagging the assignment of an unsigned int buflen to
a signed int len?

> +char *__d_path(const struct path *, const struct path *, char *, unsigned int);
> +char *d_absolute_path(const struct path *, char *, unsigned int);
> +char *d_path(const struct path *, char *, unsigned int);
> +char *dentry_path_raw(const struct dentry *, char *, unsigned int);
> +char *dentry_path(const struct dentry *, char *, unsigned int);

While you're touching these declarations, please name the 'unsigned int'
parameter.  I don't care about the others; they are obvious, but an
unsigned int might be flags, a length, or a small grey walrus.

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

[Greg Kroah-Hartman]

On Tue, Jul 27, 2021 at 12:19:55PM +0100, Matthew Wilcox wrote:
> On Tue, Jul 27, 2021 at 12:36:25PM +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.
> 
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 23a53f7b5c71..7876b741a47e 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -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);
> 
> I have questions about the quality of the analysis tool you're using.
> 
> struct prepend_buffer {
>         char *buf;
>         int len;
> };
> #define DECLARE_BUFFER(__name, __buf, __len) \
>         struct prepend_buffer __name = {.buf = __buf + __len, .len = __len}
> 
> Why is it not flagging the assignment of an unsigned int buflen to
> a signed int len?

Ah, I could not run the tool after I made this change.  I can change len
in prepend_buffer as well.

> > +char *__d_path(const struct path *, const struct path *, char *, unsigned int);
> > +char *d_absolute_path(const struct path *, char *, unsigned int);
> > +char *d_path(const struct path *, char *, unsigned int);
> > +char *dentry_path_raw(const struct dentry *, char *, unsigned int);
> > +char *dentry_path(const struct dentry *, char *, unsigned int);
> 
> While you're touching these declarations, please name the 'unsigned int'
> parameter.  I don't care about the others; they are obvious, but an
> unsigned int might be flags, a length, or a small grey walrus.

Sure, will respin this with both of those changes.  Thanks for the
review.

greg k-h

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

[Al Viro]

On Tue, Jul 27, 2021 at 12:36:25PM +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.

TBH, I'm not sure it's the right approach.  Huge argument passed to d_path()
is a bad idea, no matter what.  Do you really want to have the damn thing
try and fill 3Gb of buffer, all while holding rcu_read_lock() and a global
spinlock or two?  Hell, s/3Gb/1Gb/ and it won't get any better...


How about we do this instead:

d_path(const struct path *path, char *buf, int buflen)
{
	if (unlikely((unsigned)buflen > 0x8000)) {
		buf += (unsigned)buflen - 0x8000;
		buflen = 0x8000;
	}
	as in mainline
}

and take care of both issues?

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

[Matthew Wilcox]

On Tue, Jul 27, 2021 at 02:50:19PM +0000, Al Viro wrote:
> On Tue, Jul 27, 2021 at 12:36:25PM +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.
> 
> TBH, I'm not sure it's the right approach.  Huge argument passed to d_path()
> is a bad idea, no matter what.  Do you really want to have the damn thing
> try and fill 3Gb of buffer, all while holding rcu_read_lock() and a global
> spinlock or two?  Hell, s/3Gb/1Gb/ and it won't get any better...
> 
> 
> How about we do this instead:
> 
> d_path(const struct path *path, char *buf, int buflen)
> {
> 	if (unlikely((unsigned)buflen > 0x8000)) {
> 		buf += (unsigned)buflen - 0x8000;
> 		buflen = 0x8000;
> 	}
> 	as in mainline
> }
> 
> and take care of both issues?

umm ... what if someone passes in -ENOMEM as buflen?  Not saying we
have such a path right now, but I could imagine it happening.

	if (unlikely(buflen < 0))
		return ERR_PTR(buflen);
	if (unlikely(buflen > 0x8000)) {
		buf += buflen - 0x8000;
		buflen = 0x8000;
	}
	...

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

[Al Viro]

On Tue, Jul 27, 2021 at 04:07:47PM +0100, Matthew Wilcox wrote:

> umm ... what if someone passes in -ENOMEM as buflen?  Not saying we
> have such a path right now, but I could imagine it happening.
> 
> 	if (unlikely(buflen < 0))
> 		return ERR_PTR(buflen);
> 	if (unlikely(buflen > 0x8000)) {
> 		buf += buflen - 0x8000;
> 		buflen = 0x8000;
> 	}

Not really.  You don't want ERR_PTR() of random negative numbers to start
flying around...

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

[Matthew Wilcox]

On Tue, Jul 27, 2021 at 03:17:09PM +0000, Al Viro wrote:
> On Tue, Jul 27, 2021 at 04:07:47PM +0100, Matthew Wilcox wrote:
> 
> > umm ... what if someone passes in -ENOMEM as buflen?  Not saying we
> > have such a path right now, but I could imagine it happening.
> > 
> > 	if (unlikely(buflen < 0))
> > 		return ERR_PTR(buflen);
> > 	if (unlikely(buflen > 0x8000)) {
> > 		buf += buflen - 0x8000;
> > 		buflen = 0x8000;
> > 	}
> 
> Not really.  You don't want ERR_PTR() of random negative numbers to start
> flying around...

yeah.  the problem is that we're trying to infer what's actually going
on when the user has (potentially) passed us complete crap.  so do
we assume that 'buffer' is good if 'buflen' is >32KB?  plausible it
might be.  is it still plausibly good if buflen is >4MB?  i would say
'no'.

	if (unlikely((unsigned)buflen > 4096U * 1024))
		return ERR_PTR(-EINVAL);
	if (unlikely(buflen > 0x8000)) {
		buf += buflen - 0x8000;
		buflen = 0x8000;
	}