On Wed, 26 Oct 2016, Christos Zoulas wrote:
In article <pine.neb.4.64.1610260555490.23...@speedy.whooppee.com>,
Paul Goyette <p...@whooppee.com> wrote:
On Wed, 26 Oct 2016, matthew green wrote:
i think you're right that the 'cp' manipulation is the problem.
snprintf() will return the "desired" size, so upon the first
attempted overflow the 'cp' is moved beyond 'ep', and then the
next snprintf() gets a negative aka extremely massive value
for the buffer length and actual overflow happens here, and ssp
detects it.
the fix would be to change this:
cp += snprintf(cp, ep - cp, ...);
into this:
len = snprintf(cp, ep - cp, ...);
if (len > ep - cp)
return;
cp += len;
which is annoying because there are a lot of the former.
There's only 9 snprintf() calls. I could simply provide a macro:
#define ADD_TEXT(dest, end, format, ...) \
{ \
int len, max = (end) - (dest); \
len = snprintf((dest), max, (format), __VA_ARGS__); \
if (len > max) \
return; \
(dest) += len; \
}
Then all of the snprintf() calls become simply
ADD_TEXT(cp, ep, <format>, ...);
(Of course, after last use I'd add a #undef ADD_TEXT to clean up...)
Do you really want to stop from attaching the driver because you could
not print it's name? ...
I don't see where this would prevent the device from attaching. It would
simply return a truncated description...
... I think that perhaps a variant that returns the
number of characters appended would be more useful. like
if (len >= max)
dest = end;
else
dest += len;
but, I would leave the whole thing alone instead :-)
or write a function that appends and moves the pointer, like
snappendprintf(char **dest, const char *end, fmt, ...)
Well, as I have already pointed out to mrg@ in private Email, this
change doesn't really fix the problem anyway!
During my earlier debugging I had confirmed that the caller of
pci_devinfo() had provided a buffer of size 256, which is more than
enough to contain the entire device description. At no time did cp
point beyond the end of the buffer, not even at the very end of the
function.
The only actual "overflow" that occurs is within pci_findproduct(). It
is the call to this function that provides a PCI_PRODUCTSTR_LEN-sized
buffer. I have confirmed that pci_findproduct() properly truncates its
result and doesn't write beyond the buffer provided. pci_devinfo()
provides "char product[PCI_PRODUCTSTR_LEN]" (and similarly vendor[]).
With my specific device (0x8086, 0x6f8a),
"Core i7-6xxxK/Xeon-D Memory Cont" /* chars 0 - 31 */
"roller (Target Address, Thermal," /* chars 32 - 63 */
" RAS)" /* chars 64 - 68 */
and setting the length back to 64, the returned value was truncated (by
dev_untokenstring() in src/dev/dev_verbose.c) at the 'm' in "Thermal".
So, I'm still unclear where the stack overflow actually occurs, and
until that question is resolved, I'm not going to make any changes!
HOWEVER,
Looking at dev_untokenstring() it would appear that there is a potential
problem there! We have
cp = buf + strlcat(buf, words + *token, len - 2);
cp[0] = ' ';
cp[1] = '\0';
Since strlcat(3) (quoting the man page) "return[s] the total length of
the string [it] tried to create", it is entirely possible for cp to
point beyond the end of the buffer. Thus the insertion of a trailing
space-between-word and the NUL character could occur on some random
location.
It would seem to me the this code should be written as
newlen = strlcat(buf, word + *token, len - 2);
if (newlen > len - 2)
newlen = len - 2;
cp = buf + newlen;
cp[0] = ' ';
cp[1] = '\0';
???
+------------------+--------------------------+------------------------+
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+------------------+--------------------------+------------------------+
