On 2017/05/04 12:45PM, David Laight wrote:
> From: Naveen N. Rao [mailto:naveen.n....@linux.vnet.ibm.com]
> > Sent: 04 May 2017 11:25
> > Use safer string manipulation functions when dealing with a
> > user-provided string in kprobe_lookup_name().
> >
> > Reported-by: David Laight <david.lai...@aculab.com>
> > Signed-off-by: Naveen N. Rao <naveen.n....@linux.vnet.ibm.com>
> > ---
> > Changed to ignore return value of 0 from strscpy(), as suggested by
> > Masami.
>
> let's see what this code looks like;
>
> > char dot_name[MODULE_NAME_LEN + 1 + KSYM_NAME_LEN];
> > bool dot_appended = false;
> > + const char *c;
> > + ssize_t ret = 0;
> > + int len = 0;
> > +
> > + if ((c = strnchr(name, MODULE_NAME_LEN, ':')) != NULL) {
>
> I don't like unnecessary assignments in conditionals.
>
> > + c++;
> > + len = c - name;
> > + memcpy(dot_name, name, len);
> > + } else
> > + c = name;
> > +
> > + if (*c != '\0' && *c != '.') {
> > + dot_name[len++] = '.';
> > dot_appended = true;
>
> If you don't append a dot, then you can always just lookup
> the original string.
>
> > }
> > + ret = strscpy(dot_name + len, c, KSYM_NAME_LEN);
> > + if (ret > 0)
> > + addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
>
> I'm not sure you need 'ret' here at all.
>
> > + /* Fallback to the original non-dot symbol lookup */
> > + if (!addr && dot_appended)
> > addr = (kprobe_opcode_t *)kallsyms_lookup_name(name);
>
> We can bikeshed this function to death:
Sure, though that was never the intent. For me, this was all about
ensuring safe string manipulation. And I suppose my patch and your
version both achieve that.
>
> /* The function name must start with a '.'.
> * If it doesn't then we insert one. */
> c = strnchr(name, MODULE_NAME_LEN, ':');
> if (c && c[1] && c[1] != '.') {
^^^^^^^
while we're here, I might as well point out that that's
un-necessary and probably a few more things below...
;-)
- Naveen
> /* Insert a '.' after the ':' */
> c++;
> len = c - name;
> memcpy(dot_name, name, len);
> } else {
> if (name[0] == '.')
> goto check_name:
> /* Insert a '.' before name */
> c = name;
> len = 0;
> }
>
> dot_name[len++] = '.';
> if (strscpy(dot_name + len, c, KSYM_NAME_LEN) > 0) {
> addr = (kprobe_opcode_t *)kallsyms_lookup_name(dot_name);
> if (addr)
> return addr;
> }
> /* Symbol with extra '.' not found, fallback to original name */
>
> check_name:
> return (kprobe_opcode_t *)kallsyms_lookup_name(name);
>
> David
>
