On Sun, Apr 07, 2019 at 08:05:53AM +0000, Otcheretianski, Andrei wrote:
> > On Wed, Apr 03, 2019 at 06:17:11PM +0300, Andrei Otcheretianski wrote:
> > > The way that the P2P Device interface name was constructed, might
> > > result with an interface name that exceeds the maximal allowed
> > > interface name length (IFNAMSZ).
> > >
> > > Fix this by properly limiting the created interface name length.
> >
> > How is this supposed to work and guarantee that the truncated interface name
> > would be unique?
>
> It is not guaranteed to be unique. That's why we print a warning. In this case and in case that the driver can't handle this,
> wpa_drv_if_add() will probably return an error.
> Anyway it's a p2p device, so most probably there will be no such issue.
Maybe it would make more sense to do something else here within
wpa_supplicant to check if strlen("p2p-dev-" + current ifname) is equal
to or greater than IFNAMSIZ instead of just trying to print the string
with snprintf and truncate it at the end.
> I think IFNAMSIZ includes the NULL terminator. Though in quite a lot of places in the code it's indeed used with +1 for some reason.
> Here is a nl80211 policy as an example:
> "[NL80211_ATTR_IFNAME] = { .type = NLA_NUL_STRING, .len = IFNAMSIZ-1 },"
IFNAMSIZ has been a bit problematic in the past.. I think there was some
kernel interfaces that actually allowed an interface with 16 character
ifname to be added (while other operations did not allow it to be
removed, or something like that.. It's been a _long_ time since I last
tested this). Anyway, that nl80211 case looks pretty clear now.
> > > ret = os_snprintf(ifname, sizeof(ifname), P2P_MGMT_DEVICE_PREFIX
> > "%s",
> > > wpa_s->ifname);
> > > - if (os_snprintf_error(sizeof(ifname), ret))
> > > +
> > > + if (ret >= IFNAMSIZ) {
> > > + wpa_printf(MSG_WARNING,
> > > + "P2P: P2P Device interface name truncated=%s",
> > > + ifname);
> > > + } else if (ret < 0) {
> > > return -1;
> > > + }
> >
> > So what if snprintf return IFNAMSIZ? Wouldn't that leave ifname[] without nul
> > termination here? And that could result in reading beyond the end of the buffer
> > when using this string, e.g., in that wpa_printf() print.
>
> snprintf() never leaves strings without NULL termination. So it's ok.
Well.. snprintf() as defined in C99 might be required to do so, but
os_snprintf() maps to _snprintf() in some Windows related builds and
that has no such guarantees, which is why that os_snprintf_error()
construction got added originally. Sure, P2P Device in wpa_supplicant is
not used with Windows, so that does not happen in practice, but still, I
don't really like this assumption with os_snprintf(). Like I said above,
it would make more sense to explicitly notice that the prefix+ifname
does not fit into IFNAMSIZ and generate a different type of name based
on that instead of going through truncation with snprintf.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
