Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 11 Feb 2020 09:51:01 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Ajay.Kathat@microchip.com
Cc:     linux-wireless@vger.kernel.org, devel@driverdev.osuosl.org,
        gregkh@linuxfoundation.org, johannes@sipsolutions.net,
        Adham.Abozaeid@microchip.com
Subject: Re: [PATCH 3/3] staging: wilc1000: refactor p2p action frames
 handling API's
Message-ID: <20200211065101.GC1778@kadam>
References: <20200211000652.4781-1-ajay.kathat@microchip.com>
 <20200211000652.4781-3-ajay.kathat@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200211000652.4781-3-ajay.kathat@microchip.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 10, 2020 at 06:36:01PM +0000, Ajay.Kathat@microchip.com wrote:
> +	if (sta_ch == WILC_INVALID_CHANNEL)
> +		return;
>  
>  	while (index < len) {

This range checking was there in the original code, but it's not
correct.  index and len are in terms of bytes so we know that we can
read one byte from &buf[index] but we are reading a wilc_attr_entry
struct which is larger than a type.  The struct is actually flexibly
sized so this should be something like:

	while (index + sizeof(struct wilc_attr_entry) <= len) {
		e = (struct wilc_attr_entry *)&buf[index];
		if (index + sizeof(struct wilc_attr_entry) +
		    le16_to_cpu(e->attr_len) > len)
			break;

> -		if (buf[index] ==  CHANLIST_ATTR_ID)
> -			channel_list_attr_index = index;
> -		else if (buf[index] ==  OPERCHAN_ATTR_ID)
> -			op_channel_attr_index = index;
> -		index += buf[index + 1] + 3;
> +		e = (struct wilc_attr_entry *)&buf[index];
> +		if (e->attr_type == IEEE80211_P2P_ATTR_CHANNEL_LIST)
> +			ch_list_idx = index;
> +		else if (e->attr_type == IEEE80211_P2P_ATTR_OPER_CHANNEL)
> +			op_ch_idx = index;
> +		if (ch_list_idx && op_ch_idx)
> +			break;
> +		index += le16_to_cpu(e->attr_len) + sizeof(*e);
>  	}

regards,
dan carpenter