Bug 207786 - gpioiic_callback() dereferences IIC "how" argument incorrectly
Summary: gpioiic_callback() dereferences IIC "how" argument incorrectly
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Oleksandr Tymoshenko
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-03-08 02:35 UTC by chadf
Modified: 2019-01-15 22:42 UTC (History)
1 user (show)

See Also:


Attachments
Patch file for sys/dev/gpio/gpioiic.c (487 bytes, patch)
2016-03-08 02:35 UTC, chadf
no flags Details | Diff
gpioiic callback fix (572 bytes, patch)
2016-04-05 18:49 UTC, Oleksandr Tymoshenko
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chadf 2016-03-08 02:35:43 UTC
Created attachment 167825 [details]
Patch file for sys/dev/gpio/gpioiic.c

Instead of casting the "data" parameter to an int pointer and dereferencing, it dereferences the caddr_t (a char *) and then casts that value to an int. Currently, on little-endian systems it happens to work as expected, but is broken for other hardware.

gpioiic_callback(device_t dev, int index, caddr_t data)
{
        struct gpioiic_softc    *sc = device_get_softc(dev);
        int error, how;

        how = GPIOBUS_DONTWAIT;
        if (data != NULL && (int)*data == IIC_WAIT)
                how = GPIOBUS_WAIT;
        error = 0;
        switch (index) {
        case IIC_REQUEST_BUS:
                error = GPIOBUS_ACQUIRE_BUS(sc->sc_busdev, sc->sc_dev, how);
                break;

Patch file included.

Side note: To increase efficiency, it could be changed to an if/else setting of "how" and moved into the IIC_REQUEST_BUS switch entry, as only that code uses it.
Comment 1 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2016-04-05 18:49:38 UTC
Created attachment 169012 [details]
gpioiic callback fix

Original patch may have problem on the systems with strict alignment (MIPS, ARM) if data is not 4-byte aligned. This patch should handle this. Could you test it in your setup and let me know if it fixes original issue? 

Thanks
Comment 2 chadf 2016-04-05 23:21:32 UTC
When will anyone ever pass an address that isn't memory aligned _and_ using the interface correctly? Any code calling that which doesn't pass the address of an int (which should be aligned automatically) should be fixed instead of trying to work around it here.

The only place in the sys code that seems to initiate such a callback (for IIC_REQUEST_BUS) is iicbus_request_bus() [sys/dev/iicbus/iiconf.c], which passes the address of an int parameter that is expected to already be aligned.

The function lpbb_callback() [sys/dev/ppbus/lpbb.c] already handles this properly, presumable without any alignment problems.
Comment 3 commit-hook freebsd_committer freebsd_triage 2016-04-10 23:17:34 UTC
A commit references this bug:

Author: gonzo
Date: Sun Apr 10 23:17:06 UTC 2016
New revision: 297794
URL: https://svnweb.freebsd.org/changeset/base/297794

Log:
  Fix IIC "how" argument dereferencing on big-endian platforms

  "how" argument is passed as value of int* pointer to callback
  function but dereferenced as char* so only one byte taken into
  into account. On little-endian systems it happens to work because
  first byte is LSB that contains actual value, on big-endian it's
  MSB and in this case it's always equal zero

  PR:		207786
  Submitted by:	chadf@triularity.org

Changes:
  head/sys/dev/gpio/gpioiic.c
Comment 4 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2016-04-11 21:14:13 UTC
Yes, I agree, my solution is unnecessarily complex. I committed your fix
Comment 5 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2019-01-15 22:42:20 UTC
Closing as the submitted patch was committed in base r297794.

Thanks for reporting.