On Mon, May 26, 2014 at 09:53:56PM +0200, Konrad Zapalowicz wrote:
> -     DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFO " SIOCGIWAPLIST \n");
> -     // Only super-user can see AP list
> +     DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFO " SIOCGIWAPLIST\n");
>  
> +     /* Can we even enter the game?
> +      * 1. only super-user can see AP list
> +      * 2. pointer must be valid */

These comments are obvious, just delete them instead of reformating.

>       if (!capable(CAP_NET_ADMIN)) {
>               rc = -EPERM;
> -             return rc;
> +             goto exit;
>       }
>  
> -     if (wrq->pointer) {
> -             PKnownBSS pBSS = &(pMgmt->sBSSList[0]);
> +     if (!wrq->pointer)
> +             goto exit;
>  
> -             for (ii = 0, jj = 0; ii < MAX_BSS_NUM; ii++) {
> -                     pBSS = &(pMgmt->sBSSList[ii]);
> -                     if (!pBSS->bActive)
> -                             continue;
> -                     if (jj >= IW_MAX_AP)
> -                             break;
> -                     memcpy(sock[jj].sa_data, pBSS->abyBSSID, 6);
> -                     sock[jj].sa_family = ARPHRD_ETHER;
> -                     qual[jj].level = pBSS->uRSSI;
> -                     qual[jj].qual = qual[jj].noise = 0;
> -                     qual[jj].updated = 2;
> -                     jj++;
> -             }
> +     /* Allocate tmp tables. Must be on the heap, otherwise the
> +      * frame size is too big (exceeds 1024B) */

This comment is also pretty obvious.  Just leave it out.

> +     sock = kmalloc_array(IW_MAX_AP, sizeof(struct sockaddr), GFP_KERNEL);
> +     if (!sock) {
> +             rc = -ENOMEM;
> +             goto exit;
> +     }
> +
> +     qual = kmalloc_array(IW_MAX_AP, sizeof(struct iw_quality), GFP_KERNEL);
> +     if (!qual) {
> +             rc = -ENOMEM;
> +             goto exit;
> +     }
> +
> +     pBSS = &(pMgmt->sBSSList[0]);

No need.  This is initialized inside the loop.

> +
> +     for (ii = 0, jj = 0; ii < MAX_BSS_NUM; ii++) {
> +             pBSS = &(pMgmt->sBSSList[ii]);
> +
> +             if (!pBSS->bActive)
> +                     continue;
> +             if (jj >= IW_MAX_AP)
> +                     break;
> +
> +             s = sock + sizeof(struct sockaddr) * jj;
> +             q = qual + sizeof(struct iw_quality) * jj;

The pointer math is wrong here and will cause memory corruption.  These
are struct pointers and not void pointers, or char pointers like "extra"
is.  It should just be:

                q = qual + jj;

Or even better:

                s = &sock[jj];
                q = &qual[jj];

>  
> -             wrq->flags = 1; // Should be define'd
> -             wrq->length = jj;
> -             memcpy(extra, sock, sizeof(struct sockaddr)*jj);
> -             memcpy(extra + sizeof(struct sockaddr)*jj, qual, sizeof(struct 
> iw_quality)*jj);

Fix it up and send a v2.

regards,
dan carpenter

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to