| Message ID | 20210222151231.22572-17-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Manual replacement of all strlcpy in favor of strscpy | expand |
On Mon, 22 Feb 2021 16:12:27 +0100 Romain Perier <romain.perier@gmail.com> wrote: > The strlcpy() reads the entire source buffer first, it is dangerous if > the source buffer lenght is unbounded or possibility non NULL-terminated. > It can lead to linear read overflows, crashes, etc... > > As recommended in the deprecated interfaces [1], it should be replaced > by strscpy. > > This commit replaces all calls to strlcpy that handle the return values > by the corresponding strscpy calls with new handling of the return > values (as it is quite different between the two functions). > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > --- > kernel/trace/trace_uprobe.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index 3cf7128e1ad3..f9583afdb735 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -154,12 +154,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base) > u8 *dst = get_loc_data(dest, base); > void __user *src = (void __force __user *) addr; > > - if (unlikely(!maxlen)) > - return -ENOMEM; Don't remove the above. You just broke the else side. > - > - if (addr == FETCH_TOKEN_COMM) > - ret = strlcpy(dst, current->comm, maxlen); > - else > + if (addr == FETCH_TOKEN_COMM) { > + ret = strscpy(dst, current->comm, maxlen); > + if (ret == -E2BIG) > + return -ENOMEM; I'm not sure the above is what we want. current->comm is always nul terminated, and not only that, it will never be bigger than TASK_COMM_LEN. If the "dst" location is smaller than comm (maxlen < TASK_COMM_LEN), it is still OK to copy a partial string. It should not return -ENOMEM which looks to be what happens with this patch. In other words, it looks like this patch breaks the current code in more ways than one. -- Steve > + } else > ret = strncpy_from_user(dst, src, maxlen); > if (ret >= 0) { > if (ret == maxlen)
Le lun. 22 févr. 2021 à 18:49, Steven Rostedt <rostedt@goodmis.org> a écrit : > > - if (unlikely(!maxlen)) > > - return -ENOMEM; > > Don't remove the above. You just broke the else side. > > > - > > - if (addr == FETCH_TOKEN_COMM) > > - ret = strlcpy(dst, current->comm, maxlen); > > - else > > + if (addr == FETCH_TOKEN_COMM) { > > + ret = strscpy(dst, current->comm, maxlen); > > + if (ret == -E2BIG) > > + return -ENOMEM; > > I'm not sure the above is what we want. current->comm is always nul > terminated, and not only that, it will never be bigger than TASK_COMM_LEN. > If the "dst" location is smaller than comm (maxlen < TASK_COMM_LEN), it is > still OK to copy a partial string. It should not return -ENOMEM which looks > to be what happens with this patch. > > In other words, it looks like this patch breaks the current code in more > ways than one. > > -- Steve > Hello, Mhhh, *I think* that I had an issue during rebase, I don't remember to have removed the " if (unlikely(!maxlen))" (sorry for that). Well, strscpy always returns a truncated string even in case of possible overflow, the function copies what it can in "dst", it will just return -E2BIG when it does not fit or when "count" has a bad value (zero or > INT_MAX). We have just to make a difference between "-E2BIG, data has been copied to dst and it is truncated" and "-E2BIG, possible wrong size passed as argument". I agree that it needs at least to work like before, and I think we can preserve the old behaviour even with strscpy (we just need to adapt the error handling accordingly). I will fix this in v2. Thanks, Romain
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index 3cf7128e1ad3..f9583afdb735 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -154,12 +154,11 @@ fetch_store_string(unsigned long addr, void *dest, void *base) u8 *dst = get_loc_data(dest, base); void __user *src = (void __force __user *) addr; - if (unlikely(!maxlen)) - return -ENOMEM; - - if (addr == FETCH_TOKEN_COMM) - ret = strlcpy(dst, current->comm, maxlen); - else + if (addr == FETCH_TOKEN_COMM) { + ret = strscpy(dst, current->comm, maxlen); + if (ret == -E2BIG) + return -ENOMEM; + } else ret = strncpy_from_user(dst, src, maxlen); if (ret >= 0) { if (ret == maxlen)
The strlcpy() reads the entire source buffer first, it is dangerous if the source buffer lenght is unbounded or possibility non NULL-terminated. It can lead to linear read overflows, crashes, etc... As recommended in the deprecated interfaces [1], it should be replaced by strscpy. This commit replaces all calls to strlcpy that handle the return values by the corresponding strscpy calls with new handling of the return values (as it is quite different between the two functions). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy Signed-off-by: Romain Perier <romain.perier@gmail.com> --- kernel/trace/trace_uprobe.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)