| Message ID | 20230420192821.376828-1-krishna.t@nordicsemi.no |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] Fix use after free warning introduced by gcc 12.1 | expand |
On Fri, Apr 21, 2023 at 12:58:21AM +0530, Krishna wrote: > gcc 12.1 complains about using pointer after realloc as it could > potentially be moved/freed, causing any uses after UB. > > Fix this by doing checks before alloc and use those statuses and update > with new BSS. Could you please provide the exact warning message produced by the compiler? gcc 12.1 did not complain about anything in my test setup when compiling wpa_supplicant, so I'm assuming this would need some extra warnings to be enabled. As far as the "use of a pointer after realloc" is concerned, it should be noted that the implementation here does not dereference the old pointer after the realloc() call. What is being done here is a check of whether the buffer was indeed moved by comparing the pointer values before and after the realloc call. Is that really UB or is that an excessive warning from the compiler?
On Fri, Apr 28, 2023 at 9:09 PM Jouni Malinen <j@w1.fi> wrote: > > On Fri, Apr 21, 2023 at 12:58:21AM +0530, Krishna wrote: > > gcc 12.1 complains about using pointer after realloc as it could > > potentially be moved/freed, causing any uses after UB. > > > > Fix this by doing checks before alloc and use those statuses and update > > with new BSS. > > Could you please provide the exact warning message produced by the > compiler? gcc 12.1 did not complain about anything in my test setup when > compiling wpa_supplicant, so I'm assuming this would need some extra > warnings to be enabled. ../../../../../modules/lib/hostap/wpa_supplicant/bss.c:701:25: error: pointer 'bss' may be used after 'realloc' [-Werror=use-after-free] 701 | wpa_bss_update_pending_connect(wpa_s, bss, nbss); > > As far as the "use of a pointer after realloc" is concerned, it should > be noted that the implementation here does not dereference the old > pointer after the realloc() call. What is being done here is a > check of whether the buffer was indeed moved by comparing the pointer > values before and after the realloc call. Is that really UB or is that > an excessive warning from the compiler? Well, the code looks fine, but as the pointer after realloc might not exist, compiler can optimize the code causing indeterminate behaviour. This is discussed here https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104069 and https://www.reddit.com/r/C_Programming/comments/vffgpo/comment/icvvw8e/?utm_source=share&utm_medium=web2x&context=3
On Sun, Apr 30, 2023 at 12:04 AM Krishna Chaitanya <chaitanya.mgit@gmail.com> wrote: > > On Fri, Apr 28, 2023 at 9:09 PM Jouni Malinen <j@w1.fi> wrote: > > > > On Fri, Apr 21, 2023 at 12:58:21AM +0530, Krishna wrote: > > > gcc 12.1 complains about using pointer after realloc as it could > > > potentially be moved/freed, causing any uses after UB. > > > > > > Fix this by doing checks before alloc and use those statuses and update > > > with new BSS. > > > > Could you please provide the exact warning message produced by the > > compiler? gcc 12.1 did not complain about anything in my test setup when > > compiling wpa_supplicant, so I'm assuming this would need some extra > > warnings to be enabled. > > ../../../../../modules/lib/hostap/wpa_supplicant/bss.c:701:25: error: > pointer 'bss' may be used after 'realloc' [-Werror=use-after-free] > 701 | wpa_bss_update_pending_connect(wpa_s, bss, nbss); > > > > > As far as the "use of a pointer after realloc" is concerned, it should > > be noted that the implementation here does not dereference the old > > pointer after the realloc() call. What is being done here is a > > check of whether the buffer was indeed moved by comparing the pointer > > values before and after the realloc call. Is that really UB or is that > > an excessive warning from the compiler? > Well, the code looks fine, but as the pointer after realloc might not > exist, compiler > can optimize the code causing indeterminate behaviour. This is discussed > here https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104069 and > https://www.reddit.com/r/C_Programming/comments/vffgpo/comment/icvvw8e/?utm_source=share&utm_medium=web2x&context=3 Gentle reminder.
On Sun, Apr 30, 2023 at 12:04:57AM +0530, Krishna Chaitanya wrote: > On Fri, Apr 28, 2023 at 9:09 PM Jouni Malinen <j@w1.fi> wrote: > > > > On Fri, Apr 21, 2023 at 12:58:21AM +0530, Krishna wrote: > > > gcc 12.1 complains about using pointer after realloc as it could > > > potentially be moved/freed, causing any uses after UB. > > > > > > Fix this by doing checks before alloc and use those statuses and update > > > with new BSS. > > > > Could you please provide the exact warning message produced by the > > compiler? gcc 12.1 did not complain about anything in my test setup when > > compiling wpa_supplicant, so I'm assuming this would need some extra > > warnings to be enabled. > > ../../../../../modules/lib/hostap/wpa_supplicant/bss.c:701:25: error: > pointer 'bss' may be used after 'realloc' [-Werror=use-after-free] > 701 | wpa_bss_update_pending_connect(wpa_s, bss, nbss); Thanks for the details. This gets hidden by CONFIG_WPA_TRACE=y that I use in my builds.. I was able to reproduce this now and I applied the patch with some cleanup.
diff --git a/wpa_supplicant/bss.c b/wpa_supplicant/bss.c index 320441426..c98104ca3 100644 --- a/wpa_supplicant/bss.c +++ b/wpa_supplicant/bss.c @@ -182,9 +182,26 @@ static void wpa_bss_anqp_free(struct wpa_bss_anqp *anqp) os_free(anqp); } +static bool wpa_bss_check_pending_connect(struct wpa_supplicant *wpa_s, + struct wpa_bss *bss) +{ + struct wpa_radio_work *work; + struct wpa_connect_work *cwork; + + work = radio_work_pending(wpa_s, "sme-connect"); + if (!work) + work = radio_work_pending(wpa_s, "connect"); + if (!work) + return false; + + cwork = work->ctx; + if (cwork->bss != bss) + return false; + + return true; +} static void wpa_bss_update_pending_connect(struct wpa_supplicant *wpa_s, - struct wpa_bss *old_bss, struct wpa_bss *new_bss) { struct wpa_radio_work *work; @@ -197,8 +214,6 @@ static void wpa_bss_update_pending_connect(struct wpa_supplicant *wpa_s, return; cwork = work->ctx; - if (cwork->bss != old_bss) - return; wpa_printf(MSG_DEBUG, "Update BSS pointer for the pending connect radio work"); @@ -224,7 +239,8 @@ void wpa_bss_remove(struct wpa_supplicant *wpa_s, struct wpa_bss *bss, } } } - wpa_bss_update_pending_connect(wpa_s, bss, NULL); + if (wpa_bss_check_pending_connect(wpa_s, bss)) + wpa_bss_update_pending_connect(wpa_s, NULL); dl_list_del(&bss->list); dl_list_del(&bss->list_id); wpa_s->num_bss--; @@ -725,20 +741,24 @@ wpa_bss_update(struct wpa_supplicant *wpa_s, struct wpa_bss *bss, } else { struct wpa_bss *nbss; struct dl_list *prev = bss->list_id.prev; + bool update_pending_connect = wpa_bss_check_pending_connect( + wpa_s, bss); + unsigned int i; + bool update_current_bss = wpa_s->current_bss == bss; + for (i = 0; i < wpa_s->last_scan_res_used; i++) { + if (wpa_s->last_scan_res[i] == bss) + break; + } dl_list_del(&bss->list_id); nbss = os_realloc(bss, sizeof(*bss) + res->ie_len + res->beacon_ie_len); if (nbss) { - unsigned int i; - for (i = 0; i < wpa_s->last_scan_res_used; i++) { - if (wpa_s->last_scan_res[i] == bss) { - wpa_s->last_scan_res[i] = nbss; - break; - } - } - if (wpa_s->current_bss == bss) + if (i != wpa_s->last_scan_res_used) + wpa_s->last_scan_res[i] = nbss; + if (update_current_bss) wpa_s->current_bss = nbss; - wpa_bss_update_pending_connect(wpa_s, bss, nbss); + if (update_pending_connect) + wpa_bss_update_pending_connect(wpa_s, nbss); bss = nbss; os_memcpy(bss->ies, res + 1, res->ie_len + res->beacon_ie_len);