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
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.
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.
Solomon Peachy <[email protected]> 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
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.
Solomon Peachy <[email protected]> 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