Re: [PATCH] lib: vsprintf: Avoid 32-bit truncation in vsscanf number parsing

[Richard Fitzgerald]

On 12/11/2020 15:35, Steven Rostedt wrote:
> On Thu, 12 Nov 2020 11:17:59 +0000
> Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> > Number conversion in vsscanf converts a whole string of digits and then
> > extracts the field width part from the converted value. The maximum run
> > of digits is limited by overflow. Conversion was using either
> > simple_strto[u]l or simple_strto[u]ll based on the 'L' qualifier. This
> > created a difference in truncation between builds where long is 32-bit
> > and builds where it is 64-bit. This especially affects parsing a run of
> > contiguous digits into separate fields - the maximum length of the run
> > is 16 digits if long is 64-bit but only 8 digits if long is 32-bits.
> > For example a conversion "%6x%6x" would convert both fields correctly if
> > long is 64-bit but not if long is 32-bit.
> >
> > It is undesirable for vsscanf to parse numbers differently depending on
> > the size of long on the target build.
> >
> > As simple_strto[u]l just calls simple_strto[u]ll anyway the conversion
> > is always 64-bit, and the result is manipulated as a u64, so this is an
> > avoidable behaviour difference between 32-bit and 64-bit builds. The
> > conversion can call simple_strto[u]ll directly and preserve the full
> > 64-bits that were parsed out of the string.
> >
> > Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   lib/vsprintf.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 14c9a6af1b23..63b6cddfa7f7 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -3444,13 +3444,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
> >   			break;
> >   
> >   		if (is_sign)
> > -			val.s = qualifier != 'L' ?
> > -				simple_strtol(str, &next, base) :
> > -				simple_strtoll(str, &next, base);
> > +			val.s = simple_strtoll(str, &next, base);
> >   		else
> > -			val.u = qualifier != 'L' ?
> > -				simple_strtoul(str, &next, base) :
> > -				simple_strtoull(str, &next, base);
> > +			val.u = simple_strtoull(str, &next, base);
> >   
> >   		if (field_width > 0 && next - str > field_width) {
> >   			if (base == 0)
>
> It looks like this is fixing the symptom and not the disease. The real
> issue I see here is that vsscanf is not honoring the '6' of '%6x' here. It
> should only read the 6 characters then do the conversion, not the other
> way around! This looks to me that the design is of issue.
>
> -- Steve

See this thread from 2014 where the field width problem was raised and
explained:
http://lkml.iu.edu/hypermail/linux/kernel/1401.1/03443.html

and the reply from Linus Torvalds that was against fixing field width
handling:
http://lkml.iu.edu/hypermail/linux/kernel/1401.1/03488.html

which I assume is why the field handling wasn't unoptimized to be
strictly correct.

Nevertheless, I see no reason not to remove avoidable inconsistencies
from the current design.