| Message ID | 20170223120211.22358-1-andrew.zaborowski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
On 23 February 2017 at 13:02, Andrew Zaborowski <andrew.zaborowski@intel.com> wrote: > ieee80211_alloc_hw_nm will validate the requested name (if any) before > creating the new device and may use a name different from the one > requested rather than fail. Make sure the HWSIM_CMD_NEW_RADIO > event/response generated has the final name or userspace will receive > the wrong name. Note that mac80211_hwsim_new_radio may now modify > params. Also related to this I find that the HWSIM_ATTR_RADIO_NAME attributes emitted contain the name string and are exactly of the right length while the HWSIM_ATTR_RADIO_NAME attributes received by the kernel are assumed to be NUL-terminated. Is there a guarantee that a 0-byte follows an attribute, or should this be changed for consistency? Best regards
On Fri, 2017-02-24 at 01:08 +0100, Andrew Zaborowski wrote: > On 23 February 2017 at 13:02, Andrew Zaborowski > <andrew.zaborowski@intel.com> wrote: > > ieee80211_alloc_hw_nm will validate the requested name (if any) > > before > > creating the new device and may use a name different from the one > > requested rather than fail. Make sure the HWSIM_CMD_NEW_RADIO > > event/response generated has the final name or userspace will > > receive > > the wrong name. Note that mac80211_hwsim_new_radio may now modify > > params. > > Also related to this I find that the HWSIM_ATTR_RADIO_NAME attributes > emitted contain the name string and are exactly of the right length > while the HWSIM_ATTR_RADIO_NAME attributes received by the kernel are > assumed to be NUL-terminated. I'll agree this is a bit strange - I guess it's too late to fix now though since userspace might assume "length/data" is the string, rather than "0-terminated" (especially if there's something like python userspace where strings can trivially contain NUL bytes). nla_put_string() would have added the NUL byte. > Is there a guarantee that a 0-byte > follows an attribute, or should this be changed for consistency? [HWSIM_ATTR_RADIO_NAME] = { .type = NLA_STRING }, enforces that a NUL byte *must* be present when userspace gives us the information, so we're save - just asymmetric. Anyway, patch applied. johannes
Hi, On 27 February 2017 at 14:24, Johannes Berg <johannes@sipsolutions.net> wrote: > On Fri, 2017-02-24 at 01:08 +0100, Andrew Zaborowski wrote: >> Also related to this I find that the HWSIM_ATTR_RADIO_NAME attributes >> emitted contain the name string and are exactly of the right length >> while the HWSIM_ATTR_RADIO_NAME attributes received by the kernel are >> assumed to be NUL-terminated. > > I'll agree this is a bit strange - I guess it's too late to fix now > though since userspace might assume "length/data" is the string, rather > than "0-terminated" (especially if there's something like python > userspace where strings can trivially contain NUL bytes). > > nla_put_string() would have added the NUL byte. > >> Is there a guarantee that a 0-byte >> follows an attribute, or should this be changed for consistency? > > [HWSIM_ATTR_RADIO_NAME] = { .type = NLA_STRING }, > > enforces that a NUL byte *must* be present when userspace gives us the > information, so we're save - just asymmetric. It seems that would be NLA_NUL_STRING, whlie for NLA_STRING it's optional, I know because I didn't add the NUL in my userspace and things still worked. We can copy the received nla_data into a buffer and add the 0-byte there to support this as an option. I don't have a problem with the asymmetric usage though. > > Anyway, patch applied. Thanks Best regards
> > [HWSIM_ATTR_RADIO_NAME] = { .type = NLA_STRING }, > > > > enforces that a NUL byte *must* be present when userspace gives us > > the > > information, so we're save - just asymmetric. > > It seems that would be NLA_NUL_STRING, whlie for NLA_STRING it's > optional, Oops. I indeed misread the code there. > I know because I didn't add the NUL in my userspace and > things still worked. :) Then you probably had some 0 padding or so. > We can copy the received nla_data into a buffer > and add the 0-byte there to support this as an option. I'll do that. johannes
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 0150747..ba4978d 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -2438,6 +2438,9 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, goto failed; } + /* ieee80211_alloc_hw_nm may have used a default name */ + param->hwname = wiphy_name(hw->wiphy); + if (info) net = genl_info_net(info); else
ieee80211_alloc_hw_nm will validate the requested name (if any) before creating the new device and may use a name different from the one requested rather than fail. Make sure the HWSIM_CMD_NEW_RADIO event/response generated has the final name or userspace will receive the wrong name. Note that mac80211_hwsim_new_radio may now modify params. A check for duplicate radio name could be added separately. Signed-off-by: Andrew Zaborowski <andrew.zaborowski@intel.com> --- drivers/net/wireless/mac80211_hwsim.c | 3 +++ 1 file changed, 3 insertions(+)