| Message ID | 20200325070452.22043-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] SUNRPC: Fix a potential buffer overflow in 'svc_print_xprts()' | expand |
Hi Christophe, > On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Using 'snprintf' is safer than 'sprintf' because it can avoid a buffer > overflow. That's true as a general statement, but how likely is such an overflow to occur here? > The return value can also be used to avoid a strlen a call. That's also true of sprintf, isn't it? > Finally, we know where we need to copy and the length to copy, so, we > can save a few cycles by rearraging the code and using a memcpy instead of > a strcat. I would be OK with squashing these two patches together. I don't see the need to keep the two changes separated. > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch should have no functionnal change. > We could go further, use scnprintf and write directly in the destination > buffer. However, this could lead to a truncated last line. That's exactly what this function is trying to avoid. As part of any change in this area, it would be good to replace the current block comment before this function with a Doxygen-format comment that documents that goal. > --- > net/sunrpc/svc_xprt.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c > index df39e7b8b06c..6df861650040 100644 > --- a/net/sunrpc/svc_xprt.c > +++ b/net/sunrpc/svc_xprt.c > @@ -118,12 +118,12 @@ int svc_print_xprts(char *buf, int maxlen) > list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { > int slen; > > - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); > - slen = strlen(tmpstr); > - if (len + slen >= maxlen) > + slen = snprintf(tmpstr, sizeof(tmpstr), "%s %d\n", > + xcl->xcl_name, xcl->xcl_max_payload); > + if (slen >= sizeof(tmpstr) || len + slen >= maxlen) > break; > + memcpy(buf + len, tmpstr, slen + 1); > len += slen; > - strcat(buf, tmpstr); IMO replacing the strcat makes the code harder to read, and this is certainly not a performance path. Can you drop that part of the patch? > } > spin_unlock(&svc_xprt_class_lock); > > -- > 2.20.1 > -- Chuck Lever
On Wed, Mar 25 2020, Christophe JAILLET wrote: > Le 25/03/2020 à 15:52, Chuck Lever a écrit : >> Hi Christophe, >> >> >>> On Mar 25, 2020, at 3:04 AM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: >>> >>> Using 'snprintf' is safer than 'sprintf' because it can avoid a buffer >>> overflow. >> That's true as a general statement, but how likely is such an >> overflow to occur here? >> > I guess, that it us unlikely and that the 80 chars buffer is big enough. > That is the exact reason of why I've proposed 2 patches. The first one > could happen in RL. The 2nd is more like a clean-up and is less > relevant, IMHO. >> >>> The return value can also be used to avoid a strlen a call. >> That's also true of sprintf, isn't it? > > Sure. > > >> >>> Finally, we know where we need to copy and the length to copy, so, we >>> can save a few cycles by rearraging the code and using a memcpy instead of >>> a strcat. >> I would be OK with squashing these two patches together. I don't >> see the need to keep the two changes separated. > > NP, I can resend as a V2 with your comments. > As said above, the first fixes something that could, IMHO, happen and > the 2nd is more a matter of taste and a clean-up. > > >> >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>> --- >>> This patch should have no functionnal change. >>> We could go further, use scnprintf and write directly in the destination >>> buffer. However, this could lead to a truncated last line. >> That's exactly what this function is trying to avoid. As part of any >> change in this area, it would be good to replace the current block >> comment before this function with a Doxygen-format comment that >> documents that goal. > > I'll take care of it. > > >>> --- >>> net/sunrpc/svc_xprt.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c >>> index df39e7b8b06c..6df861650040 100644 >>> --- a/net/sunrpc/svc_xprt.c >>> +++ b/net/sunrpc/svc_xprt.c >>> @@ -118,12 +118,12 @@ int svc_print_xprts(char *buf, int maxlen) >>> list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { >>> int slen; >>> >>> - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); >>> - slen = strlen(tmpstr); >>> - if (len + slen >= maxlen) >>> + slen = snprintf(tmpstr, sizeof(tmpstr), "%s %d\n", >>> + xcl->xcl_name, xcl->xcl_max_payload); >>> + if (slen >= sizeof(tmpstr) || len + slen >= maxlen) >>> break; >>> + memcpy(buf + len, tmpstr, slen + 1); >>> len += slen; >>> - strcat(buf, tmpstr); >> IMO replacing the strcat makes the code harder to read, and this >> is certainly not a performance path. Can you drop that part of the >> patch? > > Ok > > >> >>> } >>> spin_unlock(&svc_xprt_class_lock); >>> >>> -- Can I suggest something more like this: diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index de3c077733a7..0292f45b70f6 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen) buf[0] = '\0'; spin_lock(&svc_xprt_class_lock); - list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { - int slen; - - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); - slen = strlen(tmpstr); - if (len + slen > maxlen) - break; - len += slen; - strcat(buf, tmpstr); - } + list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) + len += scnprintf(buf + len, maxlen - len, "%s %d\n", + xcl->xcl_name, xcl->xcl_max_payload); spin_unlock(&svc_xprt_class_lock); return len; NeilBrown
On Thu, Mar 26 2020, Christophe JAILLET wrote: > Le 25/03/2020 à 23:53, NeilBrown a écrit : >> Can I suggest something more like this: >> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c >> index de3c077733a7..0292f45b70f6 100644 >> --- a/net/sunrpc/svc_xprt.c >> +++ b/net/sunrpc/svc_xprt.c >> @@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen) >> buf[0] = '\0'; >> >> spin_lock(&svc_xprt_class_lock); >> - list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { >> - int slen; >> - >> - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); >> - slen = strlen(tmpstr); >> - if (len + slen > maxlen) >> - break; >> - len += slen; >> - strcat(buf, tmpstr); >> - } >> + list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) >> + len += scnprintf(buf + len, maxlen - len, "%s %d\n", >> + xcl->xcl_name, xcl->xcl_max_payload); >> spin_unlock(&svc_xprt_class_lock); >> >> return len; >> >> NeilBrown > > Hi, > > this was what I suggested in the patch: > --- > This patch should have no functional change. > We could go further, use scnprintf and write directly in the > destination > buffer. However, this could lead to a truncated last line. > --- Sorry - I missed that. So add end = strrchr(tmpstr, '\n'); if (end) end[1] = 0; else tmpstr[0] = 0; or maybe something like list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { int l = snprintf(buf + len, maxlen - len, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); if (l < maxlen - len) len += l; } buf[len] = 0; There really is no need to have the secondary buffer, and I think doing so just complicates the code. That last version is a change of behaviour in that it will skip over lines that are too long, rather than aborting on the first one. I don't know which is preferred. Thanks, NeilBrown > > And Chuck Lever confirmed that: > That's exactly what this function is trying to avoid. As part of any > change in this area, it would be good to replace the current block > comment before this function with a Doxygen-format comment that > documents that goal. > > So, I will only send a V2 based on comments already received. > > CJ
> On Mar 26, 2020, at 5:44 PM, NeilBrown <neilb@suse.de> wrote: > > On Thu, Mar 26 2020, Christophe JAILLET wrote: > >> Le 25/03/2020 à 23:53, NeilBrown a écrit : >>> Can I suggest something more like this: >>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c >>> index de3c077733a7..0292f45b70f6 100644 >>> --- a/net/sunrpc/svc_xprt.c >>> +++ b/net/sunrpc/svc_xprt.c >>> @@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen) >>> buf[0] = '\0'; >>> >>> spin_lock(&svc_xprt_class_lock); >>> - list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { >>> - int slen; >>> - >>> - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); >>> - slen = strlen(tmpstr); >>> - if (len + slen > maxlen) >>> - break; >>> - len += slen; >>> - strcat(buf, tmpstr); >>> - } >>> + list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) >>> + len += scnprintf(buf + len, maxlen - len, "%s %d\n", >>> + xcl->xcl_name, xcl->xcl_max_payload); >>> spin_unlock(&svc_xprt_class_lock); >>> >>> return len; >>> >>> NeilBrown >> >> Hi, >> >> this was what I suggested in the patch: >> --- >> This patch should have no functional change. >> We could go further, use scnprintf and write directly in the >> destination >> buffer. However, this could lead to a truncated last line. >> --- > > Sorry - I missed that. > So add > > end = strrchr(tmpstr, '\n'); > if (end) > end[1] = 0; > else > tmpstr[0] = 0; > > or maybe something like > list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { > int l = snprintf(buf + len, maxlen - len, "%s %d\n", > xcl->xcl_name, xcl->xcl_max_payload); > if (l < maxlen - len) > len += l; > } > buf[len] = 0; > > There really is no need to have the secondary buffer, and I think doing > so just complicates the code. In the interest of getting this fix into the upcoming merge window, let's stick with the temporary buffer approach. Thanks! -- Chuck Lever
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index df39e7b8b06c..6df861650040 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -118,12 +118,12 @@ int svc_print_xprts(char *buf, int maxlen) list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { int slen; - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload); - slen = strlen(tmpstr); - if (len + slen >= maxlen) + slen = snprintf(tmpstr, sizeof(tmpstr), "%s %d\n", + xcl->xcl_name, xcl->xcl_max_payload); + if (slen >= sizeof(tmpstr) || len + slen >= maxlen) break; + memcpy(buf + len, tmpstr, slen + 1); len += slen; - strcat(buf, tmpstr); } spin_unlock(&svc_xprt_class_lock);
Using 'snprintf' is safer than 'sprintf' because it can avoid a buffer overflow. The return value can also be used to avoid a strlen a call. Finally, we know where we need to copy and the length to copy, so, we can save a few cycles by rearraging the code and using a memcpy instead of a strcat. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- This patch should have no functionnal change. We could go further, use scnprintf and write directly in the destination buffer. However, this could lead to a truncated last line. --- net/sunrpc/svc_xprt.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)