Add some missing integrity checks in iicrdwr()

[jah]

Summary

iic_rdwr_data->nmsgs is uint32_t, so limit the allowable number of
messages to prevent memory exhaustion and short allocations on 32-bit
systems. Since iicrdwr is intended to be a workalike of a Linux i2c-dev
API, use the same limit of 42 that Linux uses.

Also check the return value of copyin(9) to prevent unnecessary allocation
in the failure case.

MFC after: 1 week
Submitted by: ngie

[jah]

I might've missed something, but it looks like we don't actually use the contents of buf for anything if error is set. We might store some of that bogus data in usrbufs at line 311, but we'll avoid using it.

This change does make the error handling clearer and avoids a malloc in the error case, though.

[kib]

Copy/paste from my mail to the OP:

> > $ svn diff iic.c 
> > Index: iic.c
> > ===================================================================
> > --- iic.c       (revision 295081)
> > +++ iic.c       (working copy)
> > @@ -303,6 +303,10 @@
> >         buf = malloc(sizeof(*d->msgs) * d->nmsgs, M_IIC, M_WAITOK);
> >  
> >         error = copyin(d->msgs, buf, sizeof(*d->msgs) * d->nmsgs);
> > +       if (error != 0) {
> > +               free(buf, M_IIC);
> > +               return (error);
> > +       }
> >  
> >         /* Alloc kernel buffers for userland data, copyin write data */
> >         usrbufs = malloc(sizeof(void *) * d->nmsgs, M_IIC, M_WAITOK | M_ZERO);
> > 
> This just continues the original bug.
> 
> If you look at the line above the changed line, you would see the
> multiplication with the user-supplied value, which could e.g. overflow
> and results in the short buffer being malloced. Then copying trashes the
> kernel heap.
> 
> That said, even if overflow does not occur, the user-controlled malloc(9)
> either causes DoS by over-allocation of the kernel memory, or by causing
> panic by exhausting the kmem address space on 32bit arches.

To clarify further, there are more issues in the function, I am too scared
by the function alone to look around in the file.

The lack of error-checking for copyin is somewhat mitigated later, by
checks for error == 0, but not completely, so the patch above is useful.

Still, the mallocs of length m->len are again user-controlled, and have
same issues as the allocation of buf.

If any data was transferred, error must not be returned, since this
would result in the data lost.  Instead, it should return 0 and change
failed and subsequent iic_msgs to indicate that the transfer did not
happen.

[jah]

In D5155#109705, @kib wrote:

Copy/paste from my mail to the OP:

> > $ svn diff iic.c 
> > Index: iic.c
> > ===================================================================
> > --- iic.c       (revision 295081)
> > +++ iic.c       (working copy)
> > @@ -303,6 +303,10 @@
> >         buf = malloc(sizeof(*d->msgs) * d->nmsgs, M_IIC, M_WAITOK);
> >  
> >         error = copyin(d->msgs, buf, sizeof(*d->msgs) * d->nmsgs);
> > +       if (error != 0) {
> > +               free(buf, M_IIC);
> > +               return (error);
> > +       }
> >  
> >         /* Alloc kernel buffers for userland data, copyin write data */
> >         usrbufs = malloc(sizeof(void *) * d->nmsgs, M_IIC, M_WAITOK | M_ZERO);
> > 
> This just continues the original bug.
> 
> If you look at the line above the changed line, you would see the
> multiplication with the user-supplied value, which could e.g. overflow
> and results in the short buffer being malloced. Then copying trashes the
> kernel heap.
> 
> That said, even if overflow does not occur, the user-controlled malloc(9)
> either causes DoS by over-allocation of the kernel memory, or by causing
> panic by exhausting the kmem address space on 32bit arches.

Ugh, for some reason I remembered nmsgs as being uint8_t, not uint32_t. m->len is uint16_t, which is better.
Why not either set a 255-message limit on nmsgs, or just change nmsgs to uint8_t?

Or, since this interface started out as a hack to emulate Linux's i2c-dev interface anyway, set a limit of 42 like recent Linux versions (http://lxr.free-electrons.com/source/include/uapi/linux/i2c-dev.h) ?

FWIW, nothing in the base system, not even i2c(8), uses I2CRDWR. There may be ports that use it, but I'm not aware of any. There are some i2c devices that only implement iicbus_transfer(), but most (all?) of those are controlled by kernel drivers. So perhaps it is worth considering removing this interface?

To clarify further, there are more issues in the function, I am too scared
by the function alone to look around in the file.

The lack of error-checking for copyin is somewhat mitigated later, by
checks for error == 0, but not completely, so the patch above is useful.

Still, the mallocs of length m->len are again user-controlled, and have
same issues as the allocation of buf.

If any data was transferred, error must not be returned, since this
would result in the data lost. Instead, it should return 0 and change
failed and subsequent iic_msgs to indicate that the transfer did not
happen.

I2C transfers are usually all-or-nothing. If only some of the expected data is transferred, the target device is likely in an unknown state and the only recourse is to reset it.
At that point the data really is "lost" anyway, so I don't see that the added complexity of partial-transfer handling buys us much here.

[kib]

In D5155#109758, @jah wrote:

Ugh, for some reason I remembered nmsgs as being uint8_t, not uint32_t. m->len is uint16_t, which is better.
Why not either set a 255-message limit on nmsgs, or just change nmsgs to uint8_t?

Or, since this interface started out as a hack to emulate Linux's i2c-dev interface anyway, set a limit of 42 like recent Linux versions (http://lxr.free-electrons.com/source/include/uapi/linux/i2c-dev.h) ?

I completely agree, and I tried to lead Garret to similar conclusion.

FWIW, nothing in the base system, not even i2c(8), uses I2CRDWR. There may be ports that use it, but I'm not aware of any. There are some i2c devices that only implement iicbus_transfer(), but most (all?) of those are controlled by kernel drivers. So perhaps it is worth considering removing this interface?

I do not object. Still, if this is a linux compat thing, it is better to keep the already spent efforts to be compatible.

I2C transfers are usually all-or-nothing. If only some of the expected data is transferred, the target device is likely in an unknown state and the only recourse is to reset it.
At that point the data really is "lost" anyway, so I don't see that the added complexity of partial-transfer handling buys us much here.

Ok, thank you for the clarification.

[jhb]

I concur with setting a cap on nmsgs. If Linux uses 42 then I'm fine with doing the same on FreeBSD.

[jah]

Garrett, were you planning to add the limit on nmsgs and commit this? If not, I can do it.

[ngie]

In D5155#135395, @jah wrote:

Garrett, were you planning to add the limit on nmsgs and commit this? If not, I can do it.

I don't have hardware to test with, so I shouldn't be committing more changes here. Please feel free to commandeer the revision.

[jah]

Limit number of messages to 42 to prevent exhaustion/short allocation on 32-bit systems

[kib]

This looks fine to me.

I also suggest to add a comment near malloc(m->len, ...) later in the code to note that len type is uint16_t, so that the allocation is capped at 64K.

[kib]

Another note, iic_msg handler lacks COMPAT32 shims. This is surplus, of course.

[jah]

Add comment noting size limit of each message