Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 28 Apr 2015 18:42:10 +0200 (CEST) | From | Fabian Frederick <> | Subject | Re: revert "fs/befs/linuxvfs.c: replace strncpy by strlcpy" |
| |
> On 28 April 2015 at 18:05 Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > On Tue, Apr 28, 2015 at 07:35:10AM +0200, Fabian Frederick wrote: > > > > Al, very unhappy about the prospect of looking through ~2000 calls of > > > strlcpy() > > > we have in the tree... > > > > Sorry Al, I thought it was more secure. > > It's not just you, unfortunately, and dumping all that annoyance on you > as a proxy for everyone who does that kind of thing had been unfair. > My apologies...
No problem Al :) but why can't we harden strlcpy at first with something like a strlen limited to max char. (I don't know if it's already in kernel libs).
size_t strlenl(const char *s, size_t maxlen) { const char *sc = s; size_t i = 0;
while (*sc != '\0' && (i < maxlen)) { i++; sc++; } return sc - s; }
Then we could solve problems downstream ...
Regards, Fabian
> > > I guess the 2 following patches should be reversed as well : > > > > 6cb103b6f45a > > "fs/befs/btree.c: replace strncpy by strlcpy + coding style fixing" > > > > 69201bb11327 > > "fs/ocfs2/super.c: use OCFS2_MAX_VOL_LABEL_LEN and strlcpy" > > AFAICS, they should. > > Unfortunately, we _can't_ make strlcpy() never look past src + size - 1 - > not without changing its semantics. Its callers expect it to return > the length of source; one of the intended uses is > wanted = strlcpy(dst, src, dst_size); > if (wanted >= dst_size) { > p = realloc(dst, wanted + 1); > if (!p) { > // too bad > } else { > dst = p; > memcpy(dst, src, wanted + 1); > } > } > and that really wants the accurate length. Now, the absolute majority of > strlcpy() users in the kernel completely ignore the return value, i.e. go for > silent truncation. About 1% do not. > > Out of those, some are correctly implemented "fail if truncated" uses. > The rest... Some are weirdly open-coded snprintf() (series of strlcpy and > strlcat) and some are _very_ dubious. Either they really never get > truncated, or we have a problem. For example, this > kernel/module.c:2349: s += strlcpy(s, > &mod->strtab[src[i].st_name], > smells really bad. We are truncating a bunch of strings dowsn to > KSYM_NAME_LEN > there and storing them in a buffer, with spacing that matches _untruncated_ > strings. > > drivers/s390/scsi/zfcp_fc.c:825: len = strlcpy(rspn_req->rspn.fr_name, > fc_host_symbolic_name(shost), > also looks fishy - what happens there is > len = strlcpy(rspn_req->rspn.fr_name, fc_host_symbolic_name(shost), > FC_SYMBOLIC_NAME_SIZE); > rspn_req->rspn.fr_name_len = len; > > drivers/usb/gadget/function/f_midi.c:976: result = strlcpy(page, opts->id, > PAGE_SIZE); > drivers/usb/gadget/function/f_printer.c:1172: result = strlcpy(page, > opts->pnp_string + 2, PNP_STRING_LEN - 2); > drivers/usb/gadget/function/f_printer.c:1184: result = > strlcpy(opts->pnp_string + 2, page, PNP_STRING_LEN - 2); > > are also strange...
| ![\](/images/icornerr.gif) |