Re: Usage of strncpy in the kernel

[David Holland <dholland-tech%netbsd.org@localhost>]

On Sat, Jan 04, 2025 at 12:15:11PM +0100, Johnny Billquist wrote:
 > Yikes. I wrote that too late in the night. :-)

Happens :-)

 > strncpy(buf, src, sizeof(buf));
 > buf[sizeof(buf)-1] = 0;
 > 
 > is what I should have written. Sorry.
 > 
 > But no, it only throws away the last byte of data if it don't fit into the
 > buffer, in which case that is the proper thing to do. You can't fit more
 > than N-1 bytes into an N size buffer, if you want a terminating NUL. No way
 > around that. If you want 8 actual bytes in there, then the buffer needs to
 > be 9 bytes in size (obviously).

Well yes, but then it can read past the end of the source. You need
something like

   char buf[sizeof(ut->ut_name) + 1];
   strncpy(buf, ut->ut_name, sizeof(ut->ut_name));
   buf[sizeof(ut->ut_name)] = 0;

The mere fact that we can sit here having this discussion (given that
both of us have doubtless written this exact code dozens of times) is
a sign that it's easy to get wrong.

Reading one byte off the end of a string that you're about to
overwrite with a zero anyway is not exactly a critical bug, but it
isn't necessarily just one byte, and even with one byte it can still
SIGSEGV.

 > The explicit adding of a NUL after the call could be considered a slight
 > inconvenience, but honestly, I very much do not consider that to disqualify
 > the function, and I do think if people can't get this right, then they
 > probably also do all kind of other errors, and maybe they should be put in
 > a safer place, and not poke around in the kernel.

That is true; on the other hand, people will always make errors
anyway, and one of the ways to reduce the number of people whacked in
the face is to not leave rakes lying around.

That is: all else being equal it's better to arrange your widgets so
they're resistant to predictable mistakes. If nothing else, it reduces
the number of things you have to worry about, which in turn lets you
give more attention to other concerns.

(This doesn't really lower the error rate in practice; what it means
though is that we can deal with larger and more complex systems.)

One of the things I've had the privilege to see teaching undergrads
kernel hacking for 15-odd years is that we'd find and fix little
infelicities like this and then see everyone have an easier time the
next year.

 > And I think such access should be handled already anyway if it comes
 > from userland. Users can, after all, provide broken pointers already.

Yeah, we aren't talking about copyinstr here. The reason userland
comes up is this idiom:

   struct foo {
      char txt[16];
      int count;
   };

   struct foo f; /* full of uninitialized stack data */

   strncpy(f.txt, src, sizeof(f.txt)); /* zero-fills the whole field */
   f.txt[sizeof(f.txt)-1] = 0; /* ensure null termination */
   f.count = 6; /* fill in the rest of the structure */
   err = copyout(&f, userptr, sizeof(f));

Changing that strncpy to a strlcpy will no longer zero-fill and will
therefore export kernel stack trash to userland, which is considered
an information leak even if it's mostly harmless.

 > I don't have anything against your suggested functions, though. But I'm
 > going to bet money that people would sometimes end up using the wrong one
 > in the end, and you actually didn't succeed in "solving" this problem after
 > all.

Right, of course. The goal is to smooth the path, not somehow make it
impossible to write wrong code.

You can have certain kinds of safety, to varying degrees. "Safe" is
not the same as either "secure" or "correct", but it can still be
useful.

-- 
David A. Holland
dholland%netbsd.org@localhost