Messages in this thread | | | Subject | Re: [PATCH 2.4] add SMBIOS information to /proc/smbios -- UPDATED | From | Michael Brown <> | Date | Thu, 29 Apr 2004 23:37:37 -0500 |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |