| Message ID | 20200311090555.20232-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: arm-ccn: Use scnprintf() for avoiding potential buffer overflow | expand |
On Wed, Mar 11, 2020 at 10:05:55AM +0100, Takashi Iwai wrote: > Since snprintf() returns the would-be-output size instead of the > actual output size, the succeeding calls may go beyond the given > buffer limit. Fix it by replacing with scnprintf(). The buffer limit is PAGE_SIZE bytes here, so I don't think that we can practically go beyond the buffer limit in this code where we only print tens of chacters into the buffer. On any system this runs on, PAGE_SIZE is at least 4096. I'm happy to make this change as it's generally the right thing to do, but I do want the commit message to be very clear that this is a cleanup, and not a fix for a bug in practice. Can you please either reword the commit message to that effect, or if this can occur in practice, explain how? Thanks, Mark. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > drivers/perf/arm-ccn.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c > index fea354d6fb29..cee579d428e7 100644 > --- a/drivers/perf/arm-ccn.c > +++ b/drivers/perf/arm-ccn.c > @@ -330,13 +330,13 @@ static ssize_t arm_ccn_pmu_event_show(struct device *dev, > > res = snprintf(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 >
On Fri, 13 Mar 2020 18:49:23 +0100, Mark Rutland wrote: > > On Wed, Mar 11, 2020 at 10:05:55AM +0100, Takashi Iwai wrote: > > Since snprintf() returns the would-be-output size instead of the > > actual output size, the succeeding calls may go beyond the given > > buffer limit. Fix it by replacing with scnprintf(). > > The buffer limit is PAGE_SIZE bytes here, so I don't think that we can > practically go beyond the buffer limit in this code where we only print > tens of chacters into the buffer. On any system this runs on, PAGE_SIZE > is at least 4096. Right, please take this rather as a precaution. > I'm happy to make this change as it's generally the right thing to do, > but I do want the commit message to be very clear that this is a > cleanup, and not a fix for a bug in practice. > > Can you please either reword the commit message to that effect, or if > this can occur in practice, explain how? OK, will resubmit with rephrasing. thanks, Takashi
diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c index fea354d6fb29..cee579d428e7 100644 --- a/drivers/perf/arm-ccn.c +++ b/drivers/perf/arm-ccn.c @@ -330,13 +330,13 @@ static ssize_t arm_ccn_pmu_event_show(struct device *dev, res = snprintf(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; }
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf(). Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/perf/arm-ccn.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)