Hi Jaehee,
On 4/19/22 21:19, Jaehee Park wrote:
> The free_bss_buf member of pmlmepriv is unused. Remove all related
> lines.
>
> Suggested-by: Pavel Skripkin <[email protected]>
> Signed-off-by: Jaehee Park <[email protected]>
> ---
[code snip]
> @@ -55,16 +54,6 @@ static int _rtw_init_mlme_priv(struct adapter *padapter)
>
> memset(&pmlmepriv->assoc_ssid, 0, sizeof(struct ndis_802_11_ssid));
>
> - pbuf = vzalloc(MAX_BSS_CNT * (sizeof(struct wlan_network)));
> -
> - if (!pbuf) {
> - res = _FAIL;
> - goto exit;
> - }
> - pmlmepriv->free_bss_buf = pbuf;
> -
> - pnetwork = (struct wlan_network *)pbuf;
> -
> for (i = 0; i < MAX_BSS_CNT; i++) {
> INIT_LIST_HEAD(&pnetwork->list);
>
And now kernel will just boom here :)
I am sorry for not noticing that from the beginning, that's my fault.
This pointer is used in very weird way, so let's come back to initial
patch: just remove 'if' before vfree().
Refactoring that place is the separate task
I am again sorry for wrong suggestion,
With regards,
Pavel Skripkin
As a kernel community, I don't think we are pro-active enough about
preventing bugs. One of my co-workers and I have started a bi weekly
phone call to look at CVEs from the previous week and discuss how they
could have been prevented or detected earlier. Of course, my solutions
are always centered around static analysis because that's my deal but
some bugs are caused by process issues, or could be detected with better
testing.
In this case there were two bugs proposed bugs.
1) The memory leak that Fabio noticed. Smatch is bad at detecting
memory leaks. It's a hard problem, because in this case it's
across function boundaries.
Fabio caught the leak. I don't know if I would have.
2) The NULL dereference.
The "&pnetwork->list" expression is not a dereference so this is also
a cross function thing. I thought I used to have an unpublished check
for bogus addresses like that where pnetwork is NULL.
Another is idea is that when you have pnetwork++ and it's a NULL
pointer then print an error message. Or even potentially NULL.
There are various heuristics to use which mean that "A reasonable
person would think this could be NULL".
Or another idea would be that we could test patches. Right now we
don't really have a way to test these. But, of course, we wish we
did.
It's not super likely that we would have committed the NULL deref
patch. I would have caught that one if you didn't and Fabio likely
would have as well. But I like to remove the human error whenever I
can.
regards,
dan carpenter
On Wed, Apr 20, 2022 at 02:55:34PM +0300, Dan Carpenter wrote:
> As a kernel community, I don't think we are pro-active enough about
> preventing bugs. One of my co-workers and I have started a bi weekly
> phone call to look at CVEs from the previous week and discuss how they
> could have been prevented or detected earlier. Of course, my solutions
> are always centered around static analysis because that's my deal but
> some bugs are caused by process issues, or could be detected with better
> testing.
>
> In this case there were two bugs proposed bugs.
>
> 1) The memory leak that Fabio noticed. Smatch is bad at detecting
> memory leaks. It's a hard problem, because in this case it's
> across function boundaries.
>
> Fabio caught the leak. I don't know if I would have.
>
> 2) The NULL dereference.
>
> The "&pnetwork->list" expression is not a dereference so this is also
> a cross function thing. I thought I used to have an unpublished check
> for bogus addresses like that where pnetwork is NULL.
>
> Another is idea is that when you have pnetwork++ and it's a NULL
> pointer then print an error message. Or even potentially NULL.
> There are various heuristics to use which mean that "A reasonable
> person would think this could be NULL".
>
> Or another idea would be that we could test patches. Right now we
> don't really have a way to test these. But, of course, we wish we
> did.
>
> It's not super likely that we would have committed the NULL deref
> patch. I would have caught that one if you didn't and Fabio likely
> would have as well. But I like to remove the human error whenever I
> can.
>
> regards,
> dan carpenter
I'm sorry about the NULL dereference. I wasn't sure about pnetwork
and I should've asked. I wanted to ask why pbuf should be removed
when it was being used for pnetwork but ended up not asking and
sent a faulty patch. Sorry again and thank you for explaining the
errors. I will be more careful about memory leaks and dereferencing
errors. Are there checks that I can run to detect these?
Thanks,
Jaehee
On Wed, Apr 20, 2022 at 10:48:34AM -0400, Jaehee Park wrote:
> On Wed, Apr 20, 2022 at 02:55:34PM +0300, Dan Carpenter wrote:
> > As a kernel community, I don't think we are pro-active enough about
> > preventing bugs. One of my co-workers and I have started a bi weekly
> > phone call to look at CVEs from the previous week and discuss how they
> > could have been prevented or detected earlier. Of course, my solutions
> > are always centered around static analysis because that's my deal but
> > some bugs are caused by process issues, or could be detected with better
> > testing.
> >
> > In this case there were two bugs proposed bugs.
> >
> > 1) The memory leak that Fabio noticed. Smatch is bad at detecting
> > memory leaks. It's a hard problem, because in this case it's
> > across function boundaries.
> >
> > Fabio caught the leak. I don't know if I would have.
> >
> > 2) The NULL dereference.
> >
> > The "&pnetwork->list" expression is not a dereference so this is also
> > a cross function thing. I thought I used to have an unpublished check
> > for bogus addresses like that where pnetwork is NULL.
> >
> > Another is idea is that when you have pnetwork++ and it's a NULL
> > pointer then print an error message. Or even potentially NULL.
> > There are various heuristics to use which mean that "A reasonable
> > person would think this could be NULL".
> >
> > Or another idea would be that we could test patches. Right now we
> > don't really have a way to test these. But, of course, we wish we
> > did.
> >
> > It's not super likely that we would have committed the NULL deref
> > patch. I would have caught that one if you didn't and Fabio likely
> > would have as well. But I like to remove the human error whenever I
> > can.
> >
> > regards,
> > dan carpenter
>
> I'm sorry about the NULL dereference. I wasn't sure about pnetwork
> and I should've asked. I wanted to ask why pbuf should be removed
> when it was being used for pnetwork but ended up not asking and
> sent a faulty patch. Sorry again and thank you for explaining the
> errors. I will be more careful about memory leaks and dereferencing
> errors. Are there checks that I can run to detect these?
I was going to suggest that you could write a check yourself, but then
when I looked at it it became quite complicated. :P
How about I write the check and send you the output tomorrow?
regards,
dan carpenter
On Wed, Apr 20, 2022 at 08:39:26PM +0300, Dan Carpenter wrote:
> > > Another is idea is that when you have pnetwork++ and it's a NULL
> > > pointer then print an error message. Or even potentially NULL.
> > > There are various heuristics to use which mean that "A reasonable
> > > person would think this could be NULL".
> > >
I wrote the check but it turns out none of the results are that
interesting after all.
drivers/atm/horizon.c:1844 hrz_init() warn: incrementing NULL pointer 'mem' rl='0'
drivers/scsi/qla2xxx/qla_iocb.c:665 qla24xx_build_scsi_type_6_iocbs() warn: incrementing NULL pointer 'cur_dsd' rl='4097-ptr_max'
drivers/scsi/be2iscsi/be_main.c:2686 beiscsi_init_wrb_handle() warn: incrementing NULL pointer 'pwrb' rl='0-u64max'
drivers/net/ethernet/nvidia/forcedeth.c:2304 nv_start_xmit() warn: incrementing NULL pointer 'tmp_tx_ctx' rl='0-u64max'
drivers/net/ethernet/nvidia/forcedeth.c:2482 nv_start_xmit_optimized() warn: incrementing NULL pointer 'tmp_tx_ctx' rl='0-u64max'
arch/x86/boot/cmdline.c:66 __cmdline_find_option() warn: incrementing NULL pointer 'opptr' rl='0,4096-ptr_max'
arch/x86/boot/compressed/../cmdline.c:66 __cmdline_find_option() warn: incrementing NULL pointer 'opptr' rl='0,4096-ptr_max'
arch/x86/lib/cmdline.c:84 __cmdline_find_option_bool() warn: incrementing NULL pointer 'opptr' rl='1295896293336907776,2458024882201202688,3889929980400390144,6141874131669250048,6180548506814574592,6275458169940578304,6344884530654208000'
arch/x86/lib/cmdline.c:167 __cmdline_find_option() warn: incrementing NULL pointer 'opptr' rl='0-u64max'
regards,
dan carpenter