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
