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

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]

Message-ID: <YQAClXqyLhztLcm4@kroah.com>
Date:   Tue, 27 Jul 2021 14:56:53 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jordy Zomer <jordy@...ing.systems>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH v2] fs: make d_path-like functions all have unsigned size

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