| Message ID | 1443116293-9323-12-git-send-email-ilan.peer@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Sep 24, 2015 at 08:38:01PM +0300, Ilan Peer wrote: > If a P2P provision discovery request is received for an unknown peer, > a new device entry is being added, but the flow continues without > updating the local p2p_device pointer, requiring to check the pointer > value before every access. > > Change this, so once a device is added, the flow updates the local > p2p_device pointer and avoids the checks later in the flow. > diff --git a/src/p2p/p2p_pd.c b/src/p2p/p2p_pd.c > @@ -330,8 +330,7 @@ static struct wpabuf * p2p_build_prov_disc_resp(struct p2p_data *p2p, > - if (dev) > - p2p_go_select_channel(p2p, dev, &tmp); > + p2p_go_select_channel(p2p, dev, &tmp); Could you please clarify why this change is fine? p2p_go_select_channel() dereferences the dev argument unconditionally.. > @@ -575,6 +574,19 @@ void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa, > + p2p_parse_free(&msg); > + goto out; And this "goto out" case has dev == NULL. > + if (!dev) { > + dev = p2p_get_device(p2p, sa); > + if (!dev) { > + p2p_dbg(p2p, > + "Provision Discovery device not found " > + MACSTR, MAC2STR(sa)); > + p2p_parse_free(&msg); > + goto out; > + } Just like this one.. Wouldn't this result in NULL pointer dereference in p2p_go_select_channel() due to that p2p_build_prov_disc_resp() change?
Hi Jouni, > -----Original Message----- > From: Jouni Malinen [mailto:j@w1.fi] > Sent: Monday, October 05, 2015 19:56 > To: Peer, Ilan > Cc: hostap@lists.shmoo.com > Subject: Re: [PATCH 11/23] P2P: Cleanup handling of unknown peer in PD > request processing > > On Thu, Sep 24, 2015 at 08:38:01PM +0300, Ilan Peer wrote: > > If a P2P provision discovery request is received for an unknown peer, > > a new device entry is being added, but the flow continues without > > updating the local p2p_device pointer, requiring to check the pointer > > value before every access. > > > > Change this, so once a device is added, the flow updates the local > > p2p_device pointer and avoids the checks later in the flow. > > > diff --git a/src/p2p/p2p_pd.c b/src/p2p/p2p_pd.c @@ -330,8 +330,7 @@ > > static struct wpabuf * p2p_build_prov_disc_resp(struct p2p_data *p2p, > > - if (dev) > > - p2p_go_select_channel(p2p, dev, &tmp); > > + p2p_go_select_channel(p2p, dev, &tmp); > > Could you please clarify why this change is fine? > p2p_go_select_channel() dereferences the dev argument unconditionally.. > I know see that it is not .. need to fix it. > > @@ -575,6 +574,19 @@ void p2p_process_prov_disc_req(struct p2p_data > > *p2p, const u8 *sa, > > + p2p_parse_free(&msg); > > + goto out; > > And this "goto out" case has dev == NULL. > > > + if (!dev) { > > + dev = p2p_get_device(p2p, sa); > > + if (!dev) { > > + p2p_dbg(p2p, > > + "Provision Discovery device not found > " > > + MACSTR, MAC2STR(sa)); > > + p2p_parse_free(&msg); > > + goto out; > > + } > > Just like this one.. > > Wouldn't this result in NULL pointer dereference in > p2p_go_select_channel() due to that p2p_build_prov_disc_resp() change? > Thanks. I'll fix this. Ilan.
diff --git a/src/p2p/p2p_pd.c b/src/p2p/p2p_pd.c index 1dffc89..b96b125 100644 --- a/src/p2p/p2p_pd.c +++ b/src/p2p/p2p_pd.c @@ -330,8 +330,7 @@ static struct wpabuf * p2p_build_prov_disc_resp(struct p2p_data *p2p, if (persist || (prov->conncap & P2PS_SETUP_GROUP_OWNER)) { u8 tmp; - if (dev) - p2p_go_select_channel(p2p, dev, &tmp); + p2p_go_select_channel(p2p, dev, &tmp); if (p2p->op_reg_class && p2p->op_channel) p2p_buf_add_operating_channel( @@ -575,6 +574,19 @@ void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa, 0)) { p2p_dbg(p2p, "Provision Discovery Request add device failed " MACSTR, MAC2STR(sa)); + p2p_parse_free(&msg); + goto out; + } + + if (!dev) { + dev = p2p_get_device(p2p, sa); + if (!dev) { + p2p_dbg(p2p, + "Provision Discovery device not found " + MACSTR, MAC2STR(sa)); + p2p_parse_free(&msg); + goto out; + } } } else if (msg.wfd_subelems) { wpabuf_free(dev->info.wfd_subelems); @@ -606,37 +618,33 @@ void p2p_process_prov_disc_req(struct p2p_data *p2p, const u8 *sa, } } - if (dev) { - dev->flags &= ~(P2P_DEV_PD_PEER_DISPLAY | - P2P_DEV_PD_PEER_KEYPAD | - P2P_DEV_PD_PEER_P2PS); + dev->flags &= ~(P2P_DEV_PD_PEER_DISPLAY | + P2P_DEV_PD_PEER_KEYPAD | + P2P_DEV_PD_PEER_P2PS); - /* Remove stale persistent groups */ - if (p2p->cfg->remove_stale_groups) { - p2p->cfg->remove_stale_groups( - p2p->cfg->cb_ctx, dev->info.p2p_device_addr, - msg.persistent_dev, - msg.persistent_ssid, msg.persistent_ssid_len); - } + /* Remove stale persistent groups */ + if (p2p->cfg->remove_stale_groups) { + p2p->cfg->remove_stale_groups( + p2p->cfg->cb_ctx, dev->info.p2p_device_addr, + msg.persistent_dev, + msg.persistent_ssid, msg.persistent_ssid_len); } + if (msg.wps_config_methods & WPS_CONFIG_DISPLAY) { p2p_dbg(p2p, "Peer " MACSTR " requested us to show a PIN on display", MAC2STR(sa)); - if (dev) - dev->flags |= P2P_DEV_PD_PEER_KEYPAD; + dev->flags |= P2P_DEV_PD_PEER_KEYPAD; passwd_id = DEV_PW_USER_SPECIFIED; } else if (msg.wps_config_methods & WPS_CONFIG_KEYPAD) { p2p_dbg(p2p, "Peer " MACSTR " requested us to write its PIN using keypad", MAC2STR(sa)); - if (dev) - dev->flags |= P2P_DEV_PD_PEER_DISPLAY; + dev->flags |= P2P_DEV_PD_PEER_DISPLAY; passwd_id = DEV_PW_REGISTRAR_SPECIFIED; } else if (msg.wps_config_methods & WPS_CONFIG_P2PS) { p2p_dbg(p2p, "Peer " MACSTR " requesting P2PS PIN", MAC2STR(sa)); - if (dev) - dev->flags |= P2P_DEV_PD_PEER_P2PS; + dev->flags |= P2P_DEV_PD_PEER_P2PS; passwd_id = DEV_PW_P2PS_DEFAULT; } @@ -1038,7 +1046,7 @@ out: msg.group_id, msg.group_id_len); } - if (dev && reject == P2P_SC_SUCCESS) { + if (reject == P2P_SC_SUCCESS) { switch (config_methods) { case WPS_CONFIG_DISPLAY: dev->wps_prov_info = WPS_CONFIG_KEYPAD;
If a P2P provision discovery request is received for an unknown peer, a new device entry is being added, but the flow continues without updating the local p2p_device pointer, requiring to check the pointer value before every access. Change this, so once a device is added, the flow updates the local p2p_device pointer and avoids the checks later in the flow. Signed-off-by: Ilan Peer <ilan.peer@intel.com> --- src/p2p/p2p_pd.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-)