Re: [PATCH v3 4/4] iwlwifi: mvm: add vendor commands needed for iwlmei

[Kalle Valo <kvalo@codeaurora.org>]

Emmanuel Grumbach <egrumbach@gmail.com> writes:

> On Thu, Jun 24, 2021 at 8:13 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:
>>
>> > Add the vendor commands that must be used by the network manager
>> > to allow proper operation of iwlmei.
>> >
>> > * Send information on the AP CSME is connected to
>> > * Notify the userspace when roaming is forbidden
>> > * Allow the userspace to require ownership
>> >
>> > Co-Developed-by: Ayala Beker <ayala.beker@intel.com>
>> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
>> > ---
>> >  drivers/net/wireless/intel/iwlwifi/Kconfig    |  11 ++
>> >  .../net/wireless/intel/iwlwifi/mvm/Makefile   |   1 +
>> >  .../net/wireless/intel/iwlwifi/mvm/mac80211.c |   2 +
>> >  drivers/net/wireless/intel/iwlwifi/mvm/mvm.h  |   9 +-
>> >  .../wireless/intel/iwlwifi/mvm/vendor-cmd.c   | 186 ++++++++++++++++++
>> >  5 files changed, 203 insertions(+), 6 deletions(-)
>> >  create mode 100644 drivers/net/wireless/intel/iwlwifi/mvm/vendor-cmd.c
>> >
>> > diff --git a/drivers/net/wireless/intel/iwlwifi/Kconfig b/drivers/net/wireless/intel/iwlwifi/Kconfig
>> > index 629aaa26a230..f91516d08b28 100644
>> > --- a/drivers/net/wireless/intel/iwlwifi/Kconfig
>> > +++ b/drivers/net/wireless/intel/iwlwifi/Kconfig
>> > @@ -92,11 +92,22 @@ config IWLWIFI_BCAST_FILTERING
>> >         If unsure, don't enable this option, as some programs might
>> >         expect incoming broadcasts for their normal operations.
>> >
>> > +config IWLMVM_VENDOR_CMDS
>> > +     bool "Enable vendor commands"
>> > +     depends on IWLMVM
>> > +     help
>> > +       This option enables support for vendor commands, including some
>> > +       that don't have their own Kconfig option. Other Kconfig options
>> > +       depend on this one as well.
>> > +
>> > +       This is not enabled by default, if unsure, say N.
>>
>> Why do we need a new Kconfig option? Why not always include it in the
>> compilation?
>
> I expect 99.9% of the users to want to disable this.VENDOR_CMDS adds a
> user space API and in a sense, it increases the attack surface. You
> can claim that I can reuse the IWLMEI Kconfig option, which is true,
> but we have other features that need VENDOR_CMDS that are not (yet)
> upstream. So the idea here is that any feature that needs the
> VENDOR_CMDS will select it and if none of them are enabled (for 99.9%
> of the use cases), then, we would disable VENDOR_CMDS and decrease the
> attack surface.
>
> Makes sense?

How do you prevent users or distros from enabling the feature? They can
be in a hurry, lazy or not caring and enable the feature anyway. So no,
I'm not really buying this. If the interface is not secure it should not
be in upstream, I think only exception to this is the nl80211 testmode
interface which is for lab or similar use.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches