Re: [PATCH v2] kdb: replace deprecated strncpy

[Daniel Thompson]

On Tue, Apr 09, 2024 at 01:48:38PM -0700, Justin Stitt wrote:
> Hi,
>
> On Tue, Apr 9, 2024 at 11:36 AM Daniel Thompson
> <daniel.thompson@xxxxxxxxxx> wrote:
> >
> > On Mon, Apr 08, 2024 at 05:46:42PM -0700, Justin Stitt wrote:
> > > On Fri, Apr 5, 2024 at 2:51 AM Daniel Thompson
> > > <daniel.thompson@xxxxxxxxxx> wrote:
> > > >
> > > > >                       len_tmp = strlen(p_tmp);
> > > > > -                     strncpy(cp, p_tmp+len, len_tmp-len + 1);
> > > > > +                     strscpy(cp, p_tmp+len, len_tmp-len + 1);
> > > >
> > > > Again, I really don't think the third argument provides the number of
> > > > characters in the destination buffer.
> > > >
> > >
> > > Right, the third argument is the length of the "remaining" characters
> > > from the completion point.
> >
> > Which is not how strscpy() is designed to be used.
> >
> >
> > > if you type "tes" and press tab then kallsyms_symbol_complete() will
> > > populate p_tmp with "test". Prior to rendering to the user, @cp points
> > > to "s", we need to catch the user up and print the rest of the symbol
> > > name since they've already typed "tes" we only need to print out "t".
> >
> > I'm more concerned about the case where you fill the buffer entirely
> > then move the cursor left until you get to the tes and then press Tab.
> > I think at the point we write too many bytes to cp.
> >
> >
> > > len_tmp is the length of the entire symbol part as returned by
> > > kallsyms_symbol_complete() and len is the length of only the
> > > user-typed symbol. Therefore, the amount of remaining characters to
> > > print is given by len_tmp-len (and +1 for a NUL-byte).
> > >
> > > So, yeah, you're right. This isn't the length of the destination but I
> > > don't see why we can't use memcpy() (or strscpy()) and have this not
> > > be considered "broken". The pointer arithmetic checks out.
> >
> > The problem with substituting strncpy() with memcpy() is that is *not*
> > obviously wrong... but it could be subtly wrong.
> >
> > We can see that the person who originally wrote this code made a pretty
> > serious mistake with strncpy() and the third argument if garbage. It is
> > therefore important to figure out what the *correct* value for argument
> > #3 should have been *before* we attempt to replace strncpy() with
> > anything.
> >
> > Transforming something we know to be broken without fixing it first
> > means it is impossible to know if the transformation is correct or not.
> > Hence the original question, how do we know there is enough space
> > after cp to store the string?
>
> Gotcha, I will find time to seriously refactor/rewrite this function
> (or at the very least the tab handling part of it).
>
> At the end of the day, though, I just want this strncpy() gone.

So... I starting looking into what it takes to provoke kdb_read() into
overflowing it's buffers. So far I have found two ways (one of which
does affect this strncpy() code as I thought).

I took the view that the strncpy() (or any other copy) into
tmpbuffer/p_tmp is just the wrong thing to do. A memmove() is simply
a better approach!

Short verison, whilst there is other refactoring needed, this change
fixes the overflow. I hope it also meets your ambition with respect
to strncpy().


Daniel.