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

[viro@parcelfarce.linux.theplanet.co.uk]

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;
	}

> +	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.

> +                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.

> +	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()).