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

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

[Dan Carpenter]

Hello Solomon Peachy,

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?

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.

   460                  pr_err("copy_from_user (payload %zu) failed\n", count);
   461                  return -EFAULT;
   462          }

regards,
dan carpenter

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

[Solomon Peachy]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On Tue, Jun 04, 2013 at 06:09:55AM -0700, Dan Carpenter wrote:
> 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.

> 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
-- 
Solomon Peachy        		       pizza at shaftnet dot org	 
Delray Beach, FL                          ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

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

[Kalle Valo]

Solomon Peachy <pizza@shaftnet.org> writes:

> On Tue, Jun 04, 2013 at 06:09:55AM -0700, Dan Carpenter wrote:
>> 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.
>

[...]

>
> 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.

-- 
Kalle Valo

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

[Solomon Peachy]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Wed, Jun 05, 2013 at 11:06:33AM +0300, Kalle Valo wrote:
> > 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
-- 
Solomon Peachy        		       pizza at shaftnet dot org	 
Delray Beach, FL                          ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

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

[Kalle Valo]

Solomon Peachy <pizza@shaftnet.org> writes:

> On Wed, Jun 05, 2013 at 11:06:33AM +0300, Kalle Valo wrote:
>> > 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.

This is exactly what testmode was written for.

> 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.

> 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.

-- 
Kalle Valo

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

[Solomon Peachy]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On Wed, Jun 05, 2013 at 02:36:51PM +0300, Kalle Valo wrote:
> 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.

> 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
-- 
Solomon Peachy        		       pizza at shaftnet dot org	 
Delray Beach, FL                          ^^ (email/xmpp) ^^
Quidquid latine dictum sit, altum viditur.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]