I'm in a hurry because I'm leaving for a long weekend so I've reviewed
the first easy 67 patches but this one is tricky and I'm not able to
review it properly before I leave.
On Fri, Mar 30, 2018 at 11:08:48PM -0700, Quytelda Kahja wrote:
> Copying the dummy HW address into the struct net_device doesn't need
> to be done byte by byte; use ether_addr_copy() instead.
Fine.
> Additionally, dev->dev_addr is not eight bytes long.
> ether_setup() sets the dev->addr_len to ETH_ALEN (defined as 6)
> in the net core code.
So it's a buffer overflow? The subject should have mentioned that this
is a bug fix. But I'm not sure it is. dev->dev_addr is MAX_ADDR_LEN (32)
bytes long I believe. See dev_addr_init() for details.
The commit message didn't make it clear that you were getting rid of
priv->eth_addr. It's a good change, but it confused me and I am in a
hurry.
>
> Signed-off-by: Quytelda Kahja <quytelda@xxxxxxxxxxx>
> ---
> drivers/staging/ks7010/ks_hostif.c | 29 +++++++++++------------------
> drivers/staging/ks7010/ks_wlan.h | 2 --
> drivers/staging/ks7010/ks_wlan_net.c | 13 ++-----------
> 3 files changed, 13 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
> index 1eff78540683..23c637ef147d 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -388,6 +388,7 @@ int hostif_data_indication_wpa(struct ks_wlan_private *priv,
> static
> void hostif_data_indication(struct ks_wlan_private *priv)
> {
> + struct net_device *dev = priv->net_dev;
> unsigned int rx_ind_size; /* indicate data size */
> struct sk_buff *skb;
> unsigned short auth_type;
> @@ -411,9 +412,9 @@ void hostif_data_indication(struct ks_wlan_private *priv)
> eth_proto = ntohs(eth_hdr->h_proto);
>
> /* source address check */
> - if (memcmp(&priv->eth_addr[0], eth_hdr->h_source, ETH_ALEN) == 0) {
> - netdev_err(priv->net_dev, "invalid : source is own mac address !!\n");
> - netdev_err(priv->net_dev,
> + if (memcmp(dev->dev_addr, eth_hdr->h_source, ETH_ALEN) == 0) {
> + netdev_err(dev, "invalid : source is own mac address !!\n");
> + netdev_err(dev,
> "eth_hdrernet->h_dest=%02X:%02X:%02X:%02X:%02X:%02X\n",
> eth_hdr->h_source[0], eth_hdr->h_source[1],
> eth_hdr->h_source[2], eth_hdr->h_source[3],
> @@ -443,7 +444,7 @@ void hostif_data_indication(struct ks_wlan_private *priv)
> priv->nstats.rx_dropped++;
> return;
> }
> - netdev_dbg(priv->net_dev, "SNAP, rx_ind_size = %d\n",
> + netdev_dbg(dev, "SNAP, rx_ind_size = %d\n",
> rx_ind_size);
I don't like the printk cleanups. They belong in a separate patch and
they make it harder for reviewers in a hurry to see what the patch is
doing.
Sorry, gotta run...
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
