Hi Andreas,
On Thu, 14 Feb 2019 14:00:03 +0100 Andreas Ziegler andreas.ziegler@fau.de wrote:
Hi,
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index e696667da29a..04c1769a8c7c 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -142,6 +142,13 @@ static void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs, ret = strncpy_from_user(dst, src, maxlen); if (ret == maxlen) dst[--ret] = '\0';
- else if (ret >= 0)
with this fix the maxlen case still decrements ret by one which does not include the terminating \0 (and leaves maxlen = 1 in store_trace_args()). Usually this only happens with the last element but I still feel that it's inconsistent...
Yes, I found that too. And I thought it was another fix.
Would'nt it be easier to simply add a '+ 1' to to the make_rloc_data() statement, like this:
Hmm, in that case, I would like to make "--ret" to "ret - 1" as the latest kernel does. Keeping the final code similar will help us if we need another fix to backport.
Any thought?
Thank you,
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index e696667da29a..ca0051a84ce8 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -147,7 +147,7 @@ static void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs, ((u8 *)get_rloc_data(dest))[0] = '\0'; *(u32 *)dest = make_data_rloc(0, get_rloc_offs(rloc)); } else {
*(u32 *)dest = make_data_rloc(ret, get_rloc_offs(rloc));
*(u32 *)dest = make_data_rloc(ret + 1, get_rloc_offs(rloc)); }
/** Include the terminating null byte. In this case it* was copied by strncpy_from_user but not accounted* for in ret.*/ret++;if (ret < 0) { /* Failed to fetch string */ ((u8 *)get_rloc_data(dest))[0] = '\0';
Regards,
Andreas