| Message ID | 20200315093715.8715-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 06236821aeac480a0835dd8dd9fb20e3b5a5d80d |
| Headers | show |
| Series | [v2] perf: arm-ccn: Use scnprintf() for avoiding potential buffer overflow | expand |
On Sun, Mar 15, 2020 at 10:37:15AM +0100, Takashi Iwai wrote: > snprintf() is a hard-to-use function, it's especially difficult to use > it for concatenating substrings in a buffer with a limited size. > Since snprintf() returns the would-be-output size, not the actual > size, the subsequent use of snprintf() may point to the incorrect > position easily. Although the current code doesn't actually overflow > the buffer, it's an incorrect usage. > > This patch replaces such snprintf() calls with a safer version, > scnprintf(). Thanks for clarifying the commit message. For completeness I think we should drop the final part of the commit title and say: perf: arm-ccn: use scnprintf() for robustness ... or something like that. FWIW, either way: Acked-by: Mark Rutland <mark.rutland@arm.com> Will, I assume you'll pick this up, if you're happy with that. Thanks, Mark. > > Cc: Will Deacon <will@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > v1->v2: Rephrasing the patch description. > Replace the starting snprintf(), too > > drivers/perf/arm-ccn.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c > index fea354d6fb29..d50edef91f59 100644 > --- a/drivers/perf/arm-ccn.c > +++ b/drivers/perf/arm-ccn.c > @@ -328,15 +328,15 @@ static ssize_t arm_ccn_pmu_event_show(struct device *dev, > struct arm_ccn_pmu_event, attr); > ssize_t res; > > - res = snprintf(buf, PAGE_SIZE, "type=0x%x", event->type); > + res = scnprintf(buf, PAGE_SIZE, "type=0x%x", event->type); > if (event->event) > - res += snprintf(buf + res, PAGE_SIZE - res, ",event=0x%x", > + res += scnprintf(buf + res, PAGE_SIZE - res, ",event=0x%x", > event->event); > if (event->def) > - res += snprintf(buf + res, PAGE_SIZE - res, ",%s", > + res += scnprintf(buf + res, PAGE_SIZE - res, ",%s", > event->def); > if (event->mask) > - res += snprintf(buf + res, PAGE_SIZE - res, ",mask=0x%x", > + res += scnprintf(buf + res, PAGE_SIZE - res, ",mask=0x%x", > event->mask); > > /* Arguments required by an event */ > @@ -344,25 +344,25 @@ static ssize_t arm_ccn_pmu_event_show(struct device *dev, > case CCN_TYPE_CYCLES: > break; > case CCN_TYPE_XP: > - res += snprintf(buf + res, PAGE_SIZE - res, > + res += scnprintf(buf + res, PAGE_SIZE - res, > ",xp=?,vc=?"); > if (event->event == CCN_EVENT_WATCHPOINT) > - res += snprintf(buf + res, PAGE_SIZE - res, > + res += scnprintf(buf + res, PAGE_SIZE - res, > ",port=?,dir=?,cmp_l=?,cmp_h=?,mask=?"); > else > - res += snprintf(buf + res, PAGE_SIZE - res, > + res += scnprintf(buf + res, PAGE_SIZE - res, > ",bus=?"); > > break; > case CCN_TYPE_MN: > - res += snprintf(buf + res, PAGE_SIZE - res, ",node=%d", ccn->mn_id); > + res += scnprintf(buf + res, PAGE_SIZE - res, ",node=%d", ccn->mn_id); > break; > default: > - res += snprintf(buf + res, PAGE_SIZE - res, ",node=?"); > + res += scnprintf(buf + res, PAGE_SIZE - res, ",node=?"); > break; > } > > - res += snprintf(buf + res, PAGE_SIZE - res, "\n"); > + res += scnprintf(buf + res, PAGE_SIZE - res, "\n"); > > return res; > } > -- > 2.16.4 >
diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c index fea354d6fb29..d50edef91f59 100644 --- a/drivers/perf/arm-ccn.c +++ b/drivers/perf/arm-ccn.c @@ -328,15 +328,15 @@ static ssize_t arm_ccn_pmu_event_show(struct device *dev, struct arm_ccn_pmu_event, attr); ssize_t res; - res = snprintf(buf, PAGE_SIZE, "type=0x%x", event->type); + res = scnprintf(buf, PAGE_SIZE, "type=0x%x", event->type); if (event->event) - res += snprintf(buf + res, PAGE_SIZE - res, ",event=0x%x", + res += scnprintf(buf + res, PAGE_SIZE - res, ",event=0x%x", event->event); if (event->def) - res += snprintf(buf + res, PAGE_SIZE - res, ",%s", + res += scnprintf(buf + res, PAGE_SIZE - res, ",%s", event->def); if (event->mask) - res += snprintf(buf + res, PAGE_SIZE - res, ",mask=0x%x", + res += scnprintf(buf + res, PAGE_SIZE - res, ",mask=0x%x", event->mask); /* Arguments required by an event */ @@ -344,25 +344,25 @@ static ssize_t arm_ccn_pmu_event_show(struct device *dev, case CCN_TYPE_CYCLES: break; case CCN_TYPE_XP: - res += snprintf(buf + res, PAGE_SIZE - res, + res += scnprintf(buf + res, PAGE_SIZE - res, ",xp=?,vc=?"); if (event->event == CCN_EVENT_WATCHPOINT) - res += snprintf(buf + res, PAGE_SIZE - res, + res += scnprintf(buf + res, PAGE_SIZE - res, ",port=?,dir=?,cmp_l=?,cmp_h=?,mask=?"); else - res += snprintf(buf + res, PAGE_SIZE - res, + res += scnprintf(buf + res, PAGE_SIZE - res, ",bus=?"); break; case CCN_TYPE_MN: - res += snprintf(buf + res, PAGE_SIZE - res, ",node=%d", ccn->mn_id); + res += scnprintf(buf + res, PAGE_SIZE - res, ",node=%d", ccn->mn_id); break; default: - res += snprintf(buf + res, PAGE_SIZE - res, ",node=?"); + res += scnprintf(buf + res, PAGE_SIZE - res, ",node=?"); break; } - res += snprintf(buf + res, PAGE_SIZE - res, "\n"); + res += scnprintf(buf + res, PAGE_SIZE - res, "\n"); return res; }
snprintf() is a hard-to-use function, it's especially difficult to use it for concatenating substrings in a buffer with a limited size. Since snprintf() returns the would-be-output size, not the actual size, the subsequent use of snprintf() may point to the incorrect position easily. Although the current code doesn't actually overflow the buffer, it's an incorrect usage. This patch replaces such snprintf() calls with a safer version, scnprintf(). Cc: Will Deacon <will@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Takashi Iwai <tiwai@suse.de> --- v1->v2: Rephrasing the patch description. Replace the starting snprintf(), too drivers/perf/arm-ccn.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)