On Fri, Jan 27, 2006 at 05:01:44PM -0800, Jouni Malinen wrote:
> On Thu, Jan 12, 2006 at 03:00:58PM -0500, Dan Williams wrote:
>
> > ESSIDs can technically include NULL characters. Drivers should not be
> > adjusting the length of the ESSID before reporting it in their
> > SIOCGIWESSID handlers. Breaks stuff like wpa_supplicant. Note that ipw
> > drivers, which seem to currently be the "most correct", don't have this
> > problem.
>
> I'm still trying to go through the huge amount of email I've received
> during two-week time travelling and noticed this one just now. I did not
> see any discussion about this on netdev list, but I believe the change
> here is changes the WE user space interface and this has not been done
> previously in order to not break user space programs.
>
> I agree that the choice of SSID len+1 is unfortunate for a data
> structure that is not really a string but array of octets. However, that
> was the way SIOCGIWESSID/SIOCSIWESSID was designed if I have understood
> correctly (Jean cc'ed just in case).
Yes. I admit that when I designed the API (a long time ago), I
made this mistake. It could have been worse, I could have defined it
as a pure C string (i.e. without any lenght). As it's a minor defect
and pretty easy to workaround (just add 1), I decided to leave it as
it is, to not break API compatibility.
I also made all the wtools accept both forms form
SIOCGIWESSID, so that any future change would work.
> Is this change, if applied (or was it already applied?), an indication
> that we are now changing the WE interface in a way that is not
> backwards compatible and we are willing to break an existing kernel-user
> space interface?
Good old API practices seem to be out of the window. Yes, I
would have prefered to be consulted, to change the WE number and make
sure all the bits fall in place.
> Actually, wpa_supplicant started using real SSID length, but was changed
> in September 2004 to use len+1 after Jean explained what the
> SIOCSIWESSID was designed to do..
Yes, I tried to keep the API consistent.
On Sat, Jan 28, 2006 at 12:27:53PM -0800, Jouni Malinen wrote:
> On Sat, Jan 28, 2006 at 11:07:10AM -0500, Dan Williams wrote:
>
> > 1) The patch preserves the null-termination of the ESSID, it just
> > doesn't return the expanded length to accommodate that null. So yes,
> > this may cause programs that simple copy the exact # of bytes, and then
> > do string operations on it, to crash. Programs that simply do string
> > operations on the returned essid regardless of length should be
> > unaffected. Boundary conditions are interesting here, since as the
> > ESSID approaches IW_MAX_ESSID (or whatever that is), which is 32, it was
> > unclear what the effect of padding the returned ESSID buffer was anyway.
> > If the ESSID from the AP actually was 32 bytes long, the padding null
> > would go into byte 33 and likely be ignored by the calling program if
> > it's badly coded.
>
> Agreed, I've mentioned that to Jean, but I don't remember anymore what
> was the expected way of handling 32-octet SSIDs.
I did check the handling of 32-octet ESSID in all the driver I
worked on.
> > 2) ipw and madwifi-ng at least don't do what airo/etc do. So wouldn't
> > those programs that have a problem with (1) also _already_ have a
> > problem with ipw and madwifi? Those drivers don't pad with NULL and
> > return len+1. They use the exact essid sent through SIOCSIWESSID.
> > madwifi-ng actually checks for trailing /0 and removes that before
> > setting the essid in hardware.
>
> Yes.. The "correct" behavior should be spelled out explicitly somewhere
> since there is still so much confusion about what exactly is the
> expected behavior.
It is spelled very clearly on my web site :
---------------------------------------------------------------
# The HostAP driver is the reference implementation of Wireless
Extensions for 802.11b devices, and is the reference implementation
for iwpriv functionality.
# The Aironet driver (version 1.4 and later from Ben) is an alternate
reference implementation of Wireless Extensions for 802.11b
devices. It implements fully WE-14 (Wireless Scanning and Wireless
Events), and is different than HostAP.
---------------------------------------------------------------
The problem with documentations is that they are never in sync
with the implementation, so potentially misleading. That's why we have
reference implementations.
> I think that wpa_supplicant will have to try to cope with whatever the
> kernel drivers are doing. Unfortunately this means handling multiple
> different choices. Getting the SSID from the driver is somewhat easier,
> since wpa_supplicant can just determine what is the most likely
> interpretation of the returned. Setting SSID is more difficult, since
> some drivers may expect len+1 and some require len as SSID length..
>
> Looking at your patch, it does indeed seem to be only for SIOCGIWESSID,
> so it is indeed something that will need to be merged into
> wpa_supplicant and I'll do that.
Handling both forms for SIOCGIWESSID is trivial, wtools have
been doing it for ages. Migrating SIOCSIWESSID is going to be more
painful. And having SIOCGIWESSID and SIOCSIWESSID using different
conventions is going to be confusing in the long term.
> > In any case, I don't have a problem with reverting the patch to the
> > kernel drivers, but then we either need to:
>
> Before doing any more changes, I would really like to get consensus on
> what exactly WEXT requires so that we do not need to continue changing
> this in the future. Just to be sure that we understand all cases, I
> would like to see full list of cases and values for data and length
> field for SIOCGIWESSID and SIOCSIWESSID.
WEXT is not cast in stone, it evolves with the need of drivers
and userspace. My only concern has always be to make the evolution as
painless as possible.
However, before I make a change, I always ask myself what's
the gain and what's the pain. For this one, I tought the gain was too
low.
> My current assumption is that both ioctls were expected to to include
> extra nul termination in SSID (even though I don't see much point with
> this) and the data field for SSID is expected to have that nul
> termination. In other words, if a user space program uses SSID as a
> octet string (like wpa_supplicant is doing), setting SSID would need to
> use len+1 and extra '\0' added to the end of SSID data (e.g., by
> clearing the structure before copying SSID). In case of getting the
> SSID, kernel driver would do similar changes and len-1 would be used in
> user space.
Yep.
> I would have preferred to see these ioctls use correct length and no
> expectation on the SSID data being a printable string. However, if the
> user space programs were to do this for SIOCSIWESSID, some drivers would
> configure incorrect SSID (the last octet missing).
We can make this change, it you desire.
But I would prefer two things :
o a change to WE number so that userspace can set the proper
ESSID type.
o a 6 month lead time to that new version of userspace tools
(at least iwconfig and wpa_supplicant) that handles this change have
time to propagate to most major distro (obviously we won't wait on
Debian ;-).
That's usually the way I deal with my API changes... But, this
seems no longer the way we do it around here...
> Jouni Malinen
Thanks, have fun...
Jean
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
