Re: [PATCH v2 0/4] i2c: SMBus ARP support

[Heikki Krogerus]

Hi,

> > There seems to be another a bit more severe issue with ARP and
> > i2c-dev. Right now it seems that anything that can access the i2c
> > character devices can silently (without the kernel having any idea
> > what's going on) assign a conflicting address to a dynamically
> > addressed ARP-device. Perhaps more importantly, the user space can
> > remove access to an ARP-device by silently assigning a new address to
> > it or simply by resetting its state with Prepare to ARP.
> > 
> > That can happen accidentally, but it can also be done intentionally.
> > 
> > Unless I've missed something, this really is a major threat that we
> > have to solve. Right now the only idea that I have is that we simply
> > prevent the i2c-dev from using the SMBus Default Address.
> 
> I don't necessarily see this as an issue; access to the chardev somewhat
> implies full access to the i2c bus, and so arbitrary interactions with
> devices to alter device states. Including the i2c address as part of the
> affected state doesn't seem like a huge difference in access.
> 
> There may be a specific case that is interesting though:
> 
>  - an ARP-able device is in use by a kernel driver, which excludes
>    access through the chardev
> 
>  - interactions with that device are possible using ARP commands to
>    address 0x61 over the chardev
> 
>  - so, the device may be re-addressed, now allowing accesses through
>    the chardev
> 
> However, I am not sure there are existing cases where access to the
> chardev is a distinct privilege domain to not just unbind the driver
> anyway.

Thanks for the answer. I don't think this is necessarily a major
problem either, but since it was raiced (internally) I had to make a
note. Let's not worry about this for now.

> I would have a few requests:
> 
>  - that ARP is enabled explicitly. I'd be interested in having a DT
>    property on the controller node that allows us to enable ARP on a
>    per-bus basis.
> 
>    Otherwise, I'm pretty sure we'll break someone's existing platform
>    by assuming we can start interactions on the SMBus default address.
> 
>    Is there some equivalent facility for ACPI based config?

With ACPI systems, we just don't know what the platform is going to be
used for in the end. This is a bit tricky with ACPI.

But I'll make the registration of the feature conditional somehow in
any case.

> > > Is that possible at present? how are you planning to represent ARPed
> > > devices in the DT - or more importantly, correlate DT (or other fwnode)
> > > nodes to discovered devices?
> > 
> > I don't know about DT, but with ACPI the devices are expected to
> > either be fixed address devices or just use target address that
> > matches to the address in the I2C Serial Bus Connection Resource
> > Descriptor. The mapping is not yet done, but the idea is to just
> > assign the detected UDID to the i2c-client that was already created
> > from the fwnode.
> 
> OK, but how do you map the UDID to the resource descriptor? I don't know
> much about ACPI, but the descriptor seems to be only keyed on a target
> address, which is now dynamic.
> 
> (same with DT, devices are keyed by target address)

I don't know how a dynamically addressed device, that does not return
any kind of target address with the UDID, could be mapped to the
fwnode.

> > > Assuming we go with a kernel approach: For the MCTP case, for full ARP
> > > support of MCTP endpoints, we would still need to consume a hotplug
> > > event that indicates that the device is available at its new address
> > > - there is no kernel driver bound for the remote MCTP endpoints. This
> > > event would be consumed by the (existing) MCTP infrastructure in order
> > > to start MCTP enumeration. Is this something you have looked at
> > > already? If so, if you can send an example of an actual event, I will
> > > look at the mctpd part of this.
> > 
> > We will have the address attribute file that the user space can use.
> > If the address changes uevent will be send it.
> 
> Sounds good, but for MCTP there is no struct device bound to the remote
> i2c device/address. Are you proposing we change that?

I'm sorry, I'm probable missing something here. If the address gets
changed, then you have to inform the remote endpoint about the new
address somehow in any case, right? So how did you do that with the
user space approach?

Br,

-- 
heikki