On Wed, 11 Mar 2020 17:40:02 +0100, Johan Hovold wrote: > > On Wed, Mar 11, 2020 at 12:01:26PM +0100, Takashi Iwai wrote: > > On Wed, 11 Mar 2020 11:09:03 +0100, > > Johan Hovold wrote: > > > > > > On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote: > > > > On Wed, 11 Mar 2020 10:58:14 +0100, > > > > Johan Hovold wrote: > > > > > > > > > > On Wed, Mar 11, 2020 at 10:19:06AM +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(). > > > > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > > > > > --- > > > > > > drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------ > > > > > > > > > > Thanks for the fix. > > > > > > > > > > Would you mind resending with a "staging: greybus: loopback_test:" > > > > > prefix since this is not a subsystem wide issue, bur rather a bug in a > > > > > specific user-space tool? > > > > > > > > OK, will do that. > > > > > > Thanks. > > > > > > Perhaps you should replace the snprintf() at the start of the function > > > in question as well by the way. > > > > Yeah, it's I also wonder while working on many other codes, too. > > I decided to minimize the changes at this time and concentrate only on > > the code that has a pattern like: > > pos += snprintf(buf, limit - pos, ...) > > But isn't the first snprintf() in such a sequence as much a part of the > problem as the following ones? > > If the first pos = snprintf(buf, limit, ...) overflows buf, then the > next pos += snprintf(buf, limit - pos, ...) will be called with with a > negative size argument (i.e. a very large unsigned value), which > effectively breaks the length check regardless of whether you replace it > with scnprintf() or not. And all later calls will similarly continue > writing beyond the end of buf. Yeah, that's the possible case although most calls are fine with it since the limit is PAGE_SIZE or so. This might need a bit more special care. > But wait a minute. This is user-space code, so there's no scnprintf(). > Did you not compile test this? ;P > > In fact it seems no-one has for a while. This code is just broken and > doesn't even compile any more. Maybe we should just drop it instead. Bah, I'm afraid that I overlooked this point! I've scraped over many places via a script-like work, and did the compile testing of the kernel, but not about tools. If that's the case, sorry for the mess, feel free to drop it. Takashi _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|