Re: [PATCH 2.4] add SMBIOS information to /proc/smbios -- UPDATED

Michael Brown
2004-04-30T04:38:55Z

Good stuff! Thanks for the feedback Al. Give me a few minutes and I will
send an updated patch.

Comments/Questions below.

On Thu, 2004-04-29 at 22:34, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Thu, Apr 29, 2004 at 09:21:52PM -0500, Michael Brown wrote:
> > + u32 fp = 0xF0000;
> > + while (fp < 0xFFFFF) {
> > + isa_memcpy_fromio(table_eps, fp, sizeof(*table_eps));
> > + if (memcmp(table_eps->anchor, "_SM_", 4)==0 &&
> > + checksum_eps(table_eps)) {
> > + return 0;
> > + }
> > + fp += 16;
> > + }
>
> Stilistic note:
> for (fp = 0xf0000; fp < 0xfffff; fp += 16) {
> isa_memcpy_fromio(table_eps, fp, sizeof(*table_eps));
> if (memcmp(table_eps->anchor, "_SM_", 4) != 0)
> continue;
> if (checksum_eps(table_eps))
> return 0;
> }

Will change. I like your version.

Originally copied from Alan Cox's stuff, so if Alan's style is off, oh
well... :-)

>
> > + while(keep_going && ((ptr - buf) <= max_length) && count < max_count){
> > + if (ptr[0] == 0x7F) /* ptr[0] is type */
> > + keep_going = 0;
> > +
> > + ptr += ptr[1]; /* ptr[1] is length, skip structure */
> > + /* skip strings at end of structure */
> > + while((ptr-buf) < max_length && (ptr[0] || ptr[1]))
> > + ++ptr;
>
> It looks like an off-by-one - if ptr reaches buf + max_length - 1, ptr[1]
> appears to be beyond the area it's OK to dereference.

Great spot. Updating, changed "<= max_length" to "<= (max_length-1)".

>
> > + size_t count, loff_t *ppos)
> > +{
> > + unsigned long origppos = *ppos;
> > + unsigned long max_off = the_smbios_device.smbios_table_real_length;
> > + u8 *ptr;
> > +
> > + if(*ppos >= max_off)
> > + return 0;
>
> Note that *ppos is signed here. llseek() to negative and you've got a problem.

Added "|| *ppos < 0" to the check.

>
> > + while (*ppos < max_off) {
> > + put_user(readb(ptr + *ppos), buf);
> > + ++(*ppos); ++buf;
> > + }
>
> Eeek...
>
> a) that's called copy_to_user()
> b) you'd better check the return value (either of put_user() or
> copy_to_user()).

Ok, will update, but I have one question. for (A), is this equivalent to
copy_to_user() even with the readb() in there? Sorry if this is a stupid
question.

If I get a bad return from either of these, is "return -EINVAL"
appropriate?
--
Michael