Re: [PATCH 2/5] I2O subsystem fixing and cleanup for 2.6 - i2o-passthru.patch

[Markus Lidel <Markus.Lidel@shadowconnect.com>]

Hello,

Jeff Garzik wrote:
>> +struct sg_simple_element {
>> +    u32  flag_count;
>> +    u32 addr_bus;
>> +};
> if this is truly the structure, then S/G to a 64-bit address looks 
> impossible.

The structure is only used for the management software. This is only 
used, because there is no sg_io() function for char devices like it is 
for block devices.

>> +    get_user(reply_size, &user_reply[0]);
> unchecked return val?

That's true, i'll fix this.

>> +    memset(reply, 0, REPLY_FRAME_SIZE*4);
>> +    sg_offset = (msg[0]>>4)&0x0f;
>> +    msg[2] = (u32)i2o_cfg_context;
>> +    msg[3] = (u32)reply;
> when filling in message, you should probably be using cpu_to_le32()

AFAIK the msg[2] and msg[3] is not used by the I2O controller itself. It 
is only used by the driver to track messages (when a reply message comes 
back). So it should be save to not use the cpu_to_le32() here.

>> +    memset(sg_list,0, sizeof(sg_list[0])*SG_TABLESIZE);
>> +    if(sg_offset) {
>> +        // TODO 64bit fix
>> +        struct sg_simple_element *sg = (struct sg_simple_element*) 
>> (msg+sg_offset);
>> +        sg_count = (size - sg_offset*4) / sizeof(struct 
>> sg_simple_element);
> You're de-refencing based on a userland-supplied value, without checking 
> the bounds of the variable for proper range.

Okay, i'll try fix this too.

Thank you very much that you take time to review my patch.



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com