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 >