Hi Heikki,

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.

I'm sorry if I sounded arrogant, it was not my intention.

No problem, just trying to keep things collaborative!

We don't control the user space, so we can not rely on it to enumerate
devices like this. We will not be always even able to wait for user
space with them.

Could you give some details on your intended use-case? That might help
to understand the constraints you're facing.

The kernel will also still need to be in full control of the
device, also with the ARP protocol, in order to deal with things like
conflicts. So consider for example hotplugged devices that are not
ARP-capable. If the device has a conflicting address with a
dynamically addressed ARP-device, then kernel really has to be able to
assign new address to the ARP-device completely independently.

Yeah, managing the bus addressing would definitely be simpler in-kernel.

So I think there's sufficient justification for your approach here, but
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?

 - I'd need to ensure that the i2c_client doesn't conflict with the MCTP
   transport's use of the device, post-ARP. I'll get a test setup sorted
   here, but I think that requires some changes to the controller driver
   that I'm using.

quoted

quoted

So why would you want involve the user space at all since it would
just add complexity and limitations without any benefits?

Because we have fewer risks implementing this in userspace.

As an example, you currently seem to have a stack information leak in
the proposed Get UDID implementation, which would be much less of an
issue for the equivalent protocol handling implemented in userspace.

If there are bugs in the code then we need to fix them. Can you please
comment to the patch that has the problem?

Of course, yes. I'd just like to sort the structural things before
dealing with implementation.

quoted

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)

quoted

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?

Cheers,


Jeremy