cw1200: add driver for the ST-E CW1100 & CW1200 WLAN chipsets

DISCUSSION Dan Carpenter 3 participants 6 messages Jun 4, 2013 – Jun 5, 2013

Share & tools

Solomon Peachy

2013-06-04 13:43 UTC #1 lore raw

Quoted 7 lines · Dan Carpenter

The patch a910e4a94f69: "cw1200: add driver for the ST-E CW1100 & CW1200 WLAN chipsets" from May 24, 2013, has poor input validation so the user could write to arbitrary memory.
Also I think this API looks like things which should be done with normal ioctls. This driver only lets you load the firmware using a very ugly custom debugfs interface?

No, this is a debugging interface designed to interact with the vendor-supplied testing tool and the passthrough API it requires. The vendor tool controls the device init sequence, including special engineering firmware.

Support for the ETF hooks is optional, and even if compiled in has to be explicitly enabled with a module parameter.

Quoted 10 lines

drivers/net/wireless/cw1200/debug.c
454
455 if (!count)
   456                  goto done;
   457  
   458          if (copy_from_user(etf->buf + etf->written, user_buf + written,
   459                             count)) {
"count" isn't capped so we could overwrite etf->written on the first write and then write to arbitrary memery on the second write.

Okay, that's easy enough to fix. Thanks for pointing this out.

I'll try to robustify this rather ugly interface as much as possible.

- Solomon

Signature · 3 lines

Solomon Peachy        		       pizza at shaftnet dot org	 
Delray Beach, FL                          ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.

Kalle Valo

2013-06-05 08:06 UTC #2 lore raw

Quoted 17 lines · Solomon Peachy

Quoted 7 lines · Dan Carpenter
The patch a910e4a94f69: "cw1200: add driver for the ST-E CW1100 & CW1200 WLAN chipsets" from May 24, 2013, has poor input validation so the user could write to arbitrary memory.
Also I think this API looks like things which should be done with normal ioctls. This driver only lets you load the firmware using a very ugly custom debugfs interface?
No, this is a debugging interface designed to interact with the vendor-supplied testing tool and the passthrough API it requires. The vendor tool controls the device init sequence, including special engineering firmware.
Support for the ETF hooks is optional, and even if compiled in has to be explicitly enabled with a module parameter.

[...]

Quoted 2 lines

I'll try to robustify this rather ugly interface as much as possible.

We have nl80211 testmode interface for just stuff like this. I recommend using that instead of debugfs.

Signature · 1 line

Kalle Valo

Solomon Peachy

2013-06-05 11:13 UTC #3 lore raw

Quoted 4 lines · Kalle Valo

Quoted 1 line
I'll try to robustify this rather ugly interface as much as possible.
We have nl80211 testmode interface for just stuff like this. I recommend using that instead of debugfs.

When in the so-called etf_mode, the driver doesn't create a netdev. It doesn't probe the hardware; it doesn't even (automatically) load firmware because it doesn't know what firmware to load. The driver becomes a (mostly very dumb) conduit for messages to/from userspace.

That userspace in turn is a thin daemon that listens on a TCP socket and relays everything to/from a windows-only highly proprietary vendor tool. Everything needed to initialize the hardware (even the dpll value), is obtained via these messages.

There's no inherent reason why this daemon couldn't use nl80211 instead, but IMO it's not worth the effort to rewrite the driver so it can bring up a netdev that (cleanly) fails everything other than nl80211 testcmds. After all, it'll still be an opaque binary interface whether it uses nl80211, custom ioctls, or the existing debugfs interface.

If the existing debugfs interface is too ugly to stomach in the mainline, we're better off just stripping it out and maintaining it as an out-of-tree patch for the handful of users that need to use the vendor tools -- In other words, folks bringing up new hardware designs or obtaining FCC(etc) certification.

- Solomon

Signature · 3 lines

Solomon Peachy        		       pizza at shaftnet dot org	 
Delray Beach, FL                          ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.

Kalle Valo

2013-06-05 11:36 UTC #4 lore raw

Quoted 11 lines · Solomon Peachy

Quoted 4 lines · Kalle Valo
Quoted 1 line
I'll try to robustify this rather ugly interface as much as possible.
We have nl80211 testmode interface for just stuff like this. I recommend using that instead of debugfs.
When in the so-called etf_mode, the driver doesn't create a netdev. It doesn't probe the hardware; it doesn't even (automatically) load firmware because it doesn't know what firmware to load. The driver becomes a (mostly very dumb) conduit for messages to/from userspace.

Quoted 4 lines

That userspace in turn is a thin daemon that listens on a TCP socket and relays everything to/from a windows-only highly proprietary vendor tool. Everything needed to initialize the hardware (even the dpll value), is obtained via these messages.

This is exactly what testmode was written for.

Quoted 4 lines

There's no inherent reason why this daemon couldn't use nl80211 instead, but IMO it's not worth the effort to rewrite the driver so it can bring up a netdev that (cleanly) fails everything other than nl80211 testcmds.

What do you mean with bring up netdev? I don't understand.

Even now you call ieee80211_register_hw() before cw1200_debug_init(). At least from a quick look I don't see any technical reason why you can't use testmode (unless I'm missing something).

And if there's a problem how testmode is implemented we can always fix it.

Quoted 8 lines

After all, it'll still be an opaque binary interface whether it uses nl80211, custom ioctls, or the existing debugfs interface.
If the existing debugfs interface is too ugly to stomach in the mainline, we're better off just stripping it out and maintaining it as an out-of-tree patch for the handful of users that need to use the vendor tools -- In other words, folks bringing up new hardware designs or obtaining FCC(etc) certification.

Again, this is exactly what testmode is for.

We have been working hard to get rid of all sort ugly driver specific interfaces. If you don't want to use the proper interface then I guess it's better to remove the custom debugfs interface from cw1200.

Signature · 1 line

Kalle Valo

Solomon Peachy

2013-06-05 19:12 UTC #5 lore raw

Quoted 5 lines · Kalle Valo

What do you mean with bring up netdev? I don't understand.
Even now you call ieee80211_register_hw() before cw1200_debug_init(). At least from a quick look I don't see any technical reason why you can't use testmode (unless I'm missing something).

A dozen-ish lines above the call to ieee80211_register_hw() you'll see:

 #ifdef CONFIG_CW1200_ETF
        if (etf_mode)
                goto done;

#endif

In other words, when in that debugging-only mode, the device never gets registered.

Quoted 3 lines

We have been working hard to get rid of all sort ugly driver specific interfaces. If you don't want to use the proper interface then I guess it's better to remove the custom debugfs interface from cw1200.

It's not a matter of not wanting to use the proper interface so much as having to pick my battles. (I no longer have access to the vendor tools, nor am I inclined to spend my free time hacking on something end-users aren't going to need)

I'm fine with the ETF support getting stripped out of the mainline. In the shorter term it can live as an out-of-tree patch; in the longer term it can be implemented properly using the nl80211_testmode hooks.

Meanwhile, there's a second debugfs interface ("itp") in the cw1200 driver that basically implements a continuous tx/rx function on top of regular production firmware. This should probably just go as well.

I'll prepare a couple of patches to do this.

- Solommon

Signature · 3 lines

Solomon Peachy        		       pizza at shaftnet dot org	 
Delray Beach, FL                          ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.