Re: athn(4) hostap: mem leak
On Wed, Dec 05, 2018 at 07:55:07PM +0100, Benjamin Baier wrote: > Finally got a usb athn device. I can confirm that this codepath is hit > in hostap mode and the device still works with the patch. > > athn0 at uhub4 port 2 configuration 1 interface 0 "ATHEROS USB2.0 WLAN" rev > 2.00/1.08 addr 3 > athn0: AR9271 rev 1 (1T1R), ROM rev 13, address c4:e9:84:dc:27:11 > > Full dmesg below. Committed, thanks!
Re: athn(4) hostap: mem leak
Finally got a usb athn device. I can confirm that this codepath is hit in hostap mode and the device still works with the patch. athn0 at uhub4 port 2 configuration 1 interface 0 "ATHEROS USB2.0 WLAN" rev 2.00/1.08 addr 3 athn0: AR9271 rev 1 (1T1R), ROM rev 13, address c4:e9:84:dc:27:11 Full dmesg below. On Sun, 2 Dec 2018 10:15:44 +0100 Benjamin Baier wrote: > On Sat, 1 Dec 2018 15:48:13 -0200 > Martin Pieuchot wrote: > > > On 30/11/18(Fri) 13:49, Benjamin Baier wrote: > > > Hi > > > > > > There is a leak of *arg in > > > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > > > since Rev. 1.49 > > > Because athn_usb_do_async() memcpy's the argument anyway. > > > > > > Found with llvm/scan-build. > > > > > > Instead of adding free(arg) I opted to make this function > > > more like the other ones which call athn_usb_do_async. > > > > > > Only compile tested... looking for tests. > > > > You should also remove the free(arg...) in athn_usb_newauth_cb(). > Indeed, new patch attached. Index: if_athn_usb.c === RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v retrieving revision 1.51 diff -u -p -r1.51 if_athn_usb.c --- if_athn_usb.c 6 Sep 2018 11:50:54 - 1.51 +++ if_athn_usb.c 2 Dec 2018 09:09:29 - @@ -1202,8 +1202,6 @@ athn_usb_newauth_cb(struct athn_usb_soft struct athn_node *an = (struct athn_node *)ni; int s, error = 0; - free(arg, M_DEVBUF, sizeof(*arg)); - if (ic->ic_state != IEEE80211_S_RUN) return; @@ -1231,7 +1229,7 @@ athn_usb_newauth(struct ieee80211com *ic struct ifnet *ifp = >ic_if; struct athn_node *an = (struct athn_node *)ni; int nsta; - struct athn_usb_newauth_cb_arg *arg; + struct athn_usb_newauth_cb_arg arg; if (ic->ic_opmode != IEEE80211_M_HOSTAP) return 0; @@ -1254,12 +1252,9 @@ athn_usb_newauth(struct ieee80211com *ic * In a process context, try to add this node to the * firmware table and confirm the AUTH request. */ - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); - if (arg == NULL) - return ENOMEM; - arg->ni = ieee80211_ref_node(ni); - arg->seq = seq; - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); + arg.ni = ieee80211_ref_node(ni); + arg.seq = seq; + athn_usb_do_async(usc, athn_usb_newauth_cb, , sizeof(arg)); return EBUSY; #else return 0; OpenBSD 6.4-current (GENERIC.MP) #492: Mon Dec 3 21:37:10 MST 2018 dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 8451125248 (8059MB) avail mem = 8185712640 (7806MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.6 @ 0xdae9c000 (64 entries) bios0: vendor LENOVO version "8DET69WW (1.39 )" date 07/18/2013 bios0: LENOVO 4287CTO acpi0 at bios0: rev 2 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP SLIC SSDT SSDT SSDT HPET APIC MCFG ECDT ASF! TCPA SSDT SSDT DMAR UEFI UEFI UEFI acpi0: wakeup devices LID_(S3) SLPB(S3) IGBE(S4) EXP4(S4) EXP7(S4) EHC1(S3) EHC2(S3) HDEF(S4) acpitimer0 at acpi0: 3579545 Hz, 24 bits acpihpet0 at acpi0: 14318179 Hz acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz, 2492.26 MHz, 06-2a-07 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,RDTSCP,LONG,LAHF,PERF,ITSC,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN cpu0: 256KB 64b/line 8-way L2 cache cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges cpu0: apic clock running at 99MHz cpu0: mwait min=64, max=64, C-substates=0.2.1.1.2, IBE cpu1 at mainbus0: apid 1 (application processor) cpu1: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz, 2491.91 MHz, 06-2a-07 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,RDTSCP,LONG,LAHF,PERF,ITSC,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN cpu1: 256KB 64b/line 8-way L2 cache cpu1: smt 1, core 0, package 0 cpu2 at mainbus0: apid 2 (application processor) cpu2: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz, 2491.91 MHz, 06-2a-07 cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,POPCNT,DEADLINE,AES,XSAVE,AVX,NXE,RDTSCP,LONG,LAHF,PERF,ITSC,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN cpu2:
Re: athn(4) hostap: mem leak
On Sat, 1 Dec 2018 15:48:13 -0200 Martin Pieuchot wrote: > On 30/11/18(Fri) 13:49, Benjamin Baier wrote: > > Hi > > > > There is a leak of *arg in > > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > > since Rev. 1.49 > > Because athn_usb_do_async() memcpy's the argument anyway. > > > > Found with llvm/scan-build. > > > > Instead of adding free(arg) I opted to make this function > > more like the other ones which call athn_usb_do_async. > > > > Only compile tested... looking for tests. > > You should also remove the free(arg...) in athn_usb_newauth_cb(). Indeed, new patch attached. Index: if_athn_usb.c === RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v retrieving revision 1.51 diff -u -p -r1.51 if_athn_usb.c --- if_athn_usb.c 6 Sep 2018 11:50:54 - 1.51 +++ if_athn_usb.c 2 Dec 2018 09:09:29 - @@ -1202,8 +1202,6 @@ athn_usb_newauth_cb(struct athn_usb_soft struct athn_node *an = (struct athn_node *)ni; int s, error = 0; - free(arg, M_DEVBUF, sizeof(*arg)); - if (ic->ic_state != IEEE80211_S_RUN) return; @@ -1231,7 +1229,7 @@ athn_usb_newauth(struct ieee80211com *ic struct ifnet *ifp = >ic_if; struct athn_node *an = (struct athn_node *)ni; int nsta; - struct athn_usb_newauth_cb_arg *arg; + struct athn_usb_newauth_cb_arg arg; if (ic->ic_opmode != IEEE80211_M_HOSTAP) return 0; @@ -1254,12 +1252,9 @@ athn_usb_newauth(struct ieee80211com *ic * In a process context, try to add this node to the * firmware table and confirm the AUTH request. */ - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); - if (arg == NULL) - return ENOMEM; - arg->ni = ieee80211_ref_node(ni); - arg->seq = seq; - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); + arg.ni = ieee80211_ref_node(ni); + arg.seq = seq; + athn_usb_do_async(usc, athn_usb_newauth_cb, , sizeof(arg)); return EBUSY; #else return 0;
Re: athn(4) hostap: mem leak
On 30/11/18(Fri) 13:49, Benjamin Baier wrote: > Hi > > There is a leak of *arg in > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > since Rev. 1.49 > Because athn_usb_do_async() memcpy's the argument anyway. > > Found with llvm/scan-build. > > Instead of adding free(arg) I opted to make this function > more like the other ones which call athn_usb_do_async. > > Only compile tested... looking for tests. You should also remove the free(arg...) in athn_usb_newauth_cb(). > Index: if_athn_usb.c > === > RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v > retrieving revision 1.51 > diff -u -p -r1.51 if_athn_usb.c > --- if_athn_usb.c 6 Sep 2018 11:50:54 - 1.51 > +++ if_athn_usb.c 29 Nov 2018 18:33:40 - > @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic > struct ifnet *ifp = >ic_if; > struct athn_node *an = (struct athn_node *)ni; > int nsta; > - struct athn_usb_newauth_cb_arg *arg; > + struct athn_usb_newauth_cb_arg arg; > > if (ic->ic_opmode != IEEE80211_M_HOSTAP) > return 0; > @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic >* In a process context, try to add this node to the >* firmware table and confirm the AUTH request. >*/ > - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); > - if (arg == NULL) > - return ENOMEM; > - arg->ni = ieee80211_ref_node(ni); > - arg->seq = seq; > - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); > + arg.ni = ieee80211_ref_node(ni); > + arg.seq = seq; > + athn_usb_do_async(usc, athn_usb_newauth_cb, , sizeof(arg)); > return EBUSY; > #else > return 0; >
Re: athn(4) hostap: mem leak
On Sat, Dec 01, 2018 at 10:14:38AM +0100, Benjamin Baier wrote: > On Fri, 30 Nov 2018 16:55:42 +0100 > Alexandre Ratchov wrote: > > > On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote: > > > Hi > > > > > > There is a leak of *arg in > > > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > > > since Rev. 1.49 > > > Because athn_usb_do_async() memcpy's the argument anyway. > > > > > > Found with llvm/scan-build. > > > > > > Instead of adding free(arg) I opted to make this function > > > more like the other ones which call athn_usb_do_async. > > > > > > > Hi, > > > > AFAICS, athn_usb_do_async() will schedule a call to > > athn_usb_newauth_cb(), which will use arg after the functin has > > returned. The arg memory location must stay valid after return from > > athn_usb_newauth(). So we can neither use free() nor a local variable. > > athn_usb_do_async() takes care of that by memcpy-ing arg to cmd->data > before calling usb_add_task(). > > other calls to athn_usb_do_async() do use local variables. > if_athn_usb.c:1032:athn_usb_do_async(usc, athn_usb_newstate_cb, , > sizeof(cmd)); > if_athn_usb.c:1317:athn_usb_do_async(usc, athn_usb_ampdu_tx_start_cb, , > sizeof(cmd)); > if_athn_usb.c:1641:athn_usb_do_async(usc, athn_usb_set_key_cb, , > sizeof(cmd)); > if_athn_usb.c:1673:athn_usb_do_async(usc, athn_usb_delete_key_cb, , > sizeof(cmd)); > You're right, I missed the memcpy() call, sorry. Your diff is correct.
Re: athn(4) hostap: mem leak
On Fri, 30 Nov 2018 16:55:42 +0100 Alexandre Ratchov wrote: > On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote: > > Hi > > > > There is a leak of *arg in > > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > > since Rev. 1.49 > > Because athn_usb_do_async() memcpy's the argument anyway. > > > > Found with llvm/scan-build. > > > > Instead of adding free(arg) I opted to make this function > > more like the other ones which call athn_usb_do_async. > > > > Hi, > > AFAICS, athn_usb_do_async() will schedule a call to > athn_usb_newauth_cb(), which will use arg after the functin has > returned. The arg memory location must stay valid after return from > athn_usb_newauth(). So we can neither use free() nor a local variable. athn_usb_do_async() takes care of that by memcpy-ing arg to cmd->data before calling usb_add_task(). other calls to athn_usb_do_async() do use local variables. if_athn_usb.c:1032:athn_usb_do_async(usc, athn_usb_newstate_cb, , sizeof(cmd)); if_athn_usb.c:1317:athn_usb_do_async(usc, athn_usb_ampdu_tx_start_cb, , sizeof(cmd)); if_athn_usb.c:1641:athn_usb_do_async(usc, athn_usb_set_key_cb, , sizeof(cmd)); if_athn_usb.c:1673:athn_usb_do_async(usc, athn_usb_delete_key_cb, , sizeof(cmd)); > The athn_usb_newauth_cb() callback calls free(), so there's no memory > leak unless the callback is cancelled. I don't know it can be > cancelled, I see no code doing so. > > > Only compile tested... looking for tests. > > > > Greetings Ben > > > > Index: if_athn_usb.c > > === > > RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v > > retrieving revision 1.51 > > diff -u -p -r1.51 if_athn_usb.c > > --- if_athn_usb.c 6 Sep 2018 11:50:54 - 1.51 > > +++ if_athn_usb.c 29 Nov 2018 18:33:40 - > > @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic > > struct ifnet *ifp = >ic_if; > > struct athn_node *an = (struct athn_node *)ni; > > int nsta; > > - struct athn_usb_newauth_cb_arg *arg; > > + struct athn_usb_newauth_cb_arg arg; > > > > if (ic->ic_opmode != IEEE80211_M_HOSTAP) > > return 0; > > @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic > > * In a process context, try to add this node to the > > * firmware table and confirm the AUTH request. > > */ > > - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); > > - if (arg == NULL) > > - return ENOMEM; > > - arg->ni = ieee80211_ref_node(ni); > > - arg->seq = seq; > > - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); > > + arg.ni = ieee80211_ref_node(ni); > > + arg.seq = seq; > > + athn_usb_do_async(usc, athn_usb_newauth_cb, , sizeof(arg)); > > return EBUSY; > > #else > > return 0; > > >
Re: athn(4) hostap: mem leak
On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote: > Hi > > There is a leak of *arg in > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > since Rev. 1.49 > Because athn_usb_do_async() memcpy's the argument anyway. > > Found with llvm/scan-build. > > Instead of adding free(arg) I opted to make this function > more like the other ones which call athn_usb_do_async. > Hi, AFAICS, athn_usb_do_async() will schedule a call to athn_usb_newauth_cb(), which will use arg after the functin has returned. The arg memory location must stay valid after return from athn_usb_newauth(). So we can neither use free() nor a local variable. The athn_usb_newauth_cb() callback calls free(), so there's no memory leak unless the callback is cancelled. I don't know it can be cancelled, I see no code doing so. > Only compile tested... looking for tests. > > Greetings Ben > > Index: if_athn_usb.c > === > RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v > retrieving revision 1.51 > diff -u -p -r1.51 if_athn_usb.c > --- if_athn_usb.c 6 Sep 2018 11:50:54 - 1.51 > +++ if_athn_usb.c 29 Nov 2018 18:33:40 - > @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic > struct ifnet *ifp = >ic_if; > struct athn_node *an = (struct athn_node *)ni; > int nsta; > - struct athn_usb_newauth_cb_arg *arg; > + struct athn_usb_newauth_cb_arg arg; > > if (ic->ic_opmode != IEEE80211_M_HOSTAP) > return 0; > @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic >* In a process context, try to add this node to the >* firmware table and confirm the AUTH request. >*/ > - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); > - if (arg == NULL) > - return ENOMEM; > - arg->ni = ieee80211_ref_node(ni); > - arg->seq = seq; > - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); > + arg.ni = ieee80211_ref_node(ni); > + arg.seq = seq; > + athn_usb_do_async(usc, athn_usb_newauth_cb, , sizeof(arg)); > return EBUSY; > #else > return 0; > --
Re: athn(4) hostap: mem leak
On Fri, Nov 30, 2018 at 01:49:56PM +0100, Benjamin Baier wrote: > Hi > > There is a leak of *arg in > dev/usb/if_athn_usb.c:athn_usb_newauth() line 1263 > since Rev. 1.49 > Because athn_usb_do_async() memcpy's the argument anyway. > > Found with llvm/scan-build. > > Instead of adding free(arg) I opted to make this function > more like the other ones which call athn_usb_do_async. > > Only compile tested... looking for tests. > > Greetings Ben Looks good to me. I would appreciate someone else testing this, too. > > Index: if_athn_usb.c > === > RCS file: /cvs/src/sys/dev/usb/if_athn_usb.c,v > retrieving revision 1.51 > diff -u -p -r1.51 if_athn_usb.c > --- if_athn_usb.c 6 Sep 2018 11:50:54 - 1.51 > +++ if_athn_usb.c 29 Nov 2018 18:33:40 - > @@ -1231,7 +1231,7 @@ athn_usb_newauth(struct ieee80211com *ic > struct ifnet *ifp = >ic_if; > struct athn_node *an = (struct athn_node *)ni; > int nsta; > - struct athn_usb_newauth_cb_arg *arg; > + struct athn_usb_newauth_cb_arg arg; > > if (ic->ic_opmode != IEEE80211_M_HOSTAP) > return 0; > @@ -1254,12 +1254,9 @@ athn_usb_newauth(struct ieee80211com *ic >* In a process context, try to add this node to the >* firmware table and confirm the AUTH request. >*/ > - arg = malloc(sizeof(*arg), M_DEVBUF, M_NOWAIT); > - if (arg == NULL) > - return ENOMEM; > - arg->ni = ieee80211_ref_node(ni); > - arg->seq = seq; > - athn_usb_do_async(usc, athn_usb_newauth_cb, arg, sizeof(*arg)); > + arg.ni = ieee80211_ref_node(ni); > + arg.seq = seq; > + athn_usb_do_async(usc, athn_usb_newauth_cb, , sizeof(arg)); > return EBUSY; > #else > return 0; >
