netdev - Re: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+h21hq9rgvQzjYO5bbxXs9wmVy-1Fomnre+veUS_+P-mwLUdg@mail.gmail.com>
Date:   Tue, 26 Nov 2019 14:31:35 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Yangbo Lu <yangbo.lu@....com>
Cc:     netdev <netdev@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH] net: mscc: ocelot: fix potential issues accessing skbs list

Hi Yangbo,

On Tue, 26 Nov 2019 at 14:22, Yangbo Lu <yangbo.lu@....com> wrote:
>
> Fix two prtential issues accessing skbs list.
> - Protect skbs list in case of competing for accessing.
> - Break the matching loop when find the matching skb to
>   avoid consuming more skbs incorrectly. The ID is only
>   from 0 to 3, but the FIFO supports 128 timestamps at most.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@....com>
> ---

Don't you want to convert struct ocelot_skb to a proper struct
sk_buff_head skb_queue, which has its own lock when calling
skb_dequeue? The timestamp ID can be held in skb->cb.

>  drivers/net/ethernet/mscc/ocelot.c | 11 +++++++++++
>  include/soc/mscc/ocelot.h          |  2 ++
>  2 files changed, 13 insertions(+)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 0e96ffa..5d842d8 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -580,6 +580,7 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
>  {
>         struct skb_shared_info *shinfo = skb_shinfo(skb);
>         struct ocelot *ocelot = ocelot_port->ocelot;
> +       unsigned long flags;
>
>         if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
>             ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> @@ -594,7 +595,9 @@ int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
>                 oskb->skb = skb;
>                 oskb->id = ocelot_port->ts_id % 4;
>
> +               spin_lock_irqsave(&ocelot_port->skbs_lock, flags);
>                 list_add_tail(&oskb->head, &ocelot_port->skbs);
> +               spin_unlock_irqrestore(&ocelot_port->skbs_lock, flags);
>                 return 0;
>         }
>         return -ENODATA;
> @@ -702,6 +705,7 @@ static void ocelot_get_hwtimestamp(struct ocelot *ocelot,
>  void ocelot_get_txtstamp(struct ocelot *ocelot)
>  {
>         int budget = OCELOT_PTP_QUEUE_SZ;
> +       unsigned long flags;
>
>         while (budget--) {
>                 struct skb_shared_hwtstamps shhwtstamps;
> @@ -727,6 +731,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
>                 /* Retrieve its associated skb */
>                 port = ocelot->ports[txport];
>
> +               spin_lock_irqsave(&port->skbs_lock, flags);
>                 list_for_each_safe(pos, tmp, &port->skbs) {
>                         entry = list_entry(pos, struct ocelot_skb, head);
>                         if (entry->id != id)
> @@ -736,7 +741,9 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
>
>                         list_del(pos);
>                         kfree(entry);
> +                       break;

I think this should be a separate patch, since it is unrelated to locking.

>                 }
> +               spin_unlock_irqrestore(&port->skbs_lock, flags);
>
>                 /* Next ts */
>                 ocelot_write(ocelot, SYS_PTP_NXT_PTP_NXT, SYS_PTP_NXT);
> @@ -2205,6 +2212,7 @@ void ocelot_init_port(struct ocelot *ocelot, int port)
>  {
>         struct ocelot_port *ocelot_port = ocelot->ports[port];
>
> +       spin_lock_init(&ocelot_port->skbs_lock);
>         INIT_LIST_HEAD(&ocelot_port->skbs);
>
>         /* Basic L2 initialization */
> @@ -2493,6 +2501,7 @@ void ocelot_deinit(struct ocelot *ocelot)
>         struct list_head *pos, *tmp;
>         struct ocelot_port *port;
>         struct ocelot_skb *entry;
> +       unsigned long flags;
>         int i;
>
>         cancel_delayed_work(&ocelot->stats_work);
> @@ -2503,6 +2512,7 @@ void ocelot_deinit(struct ocelot *ocelot)
>         for (i = 0; i < ocelot->num_phys_ports; i++) {
>                 port = ocelot->ports[i];
>
> +               spin_lock_irqsave(&port->skbs_lock, flags);
>                 list_for_each_safe(pos, tmp, &port->skbs) {
>                         entry = list_entry(pos, struct ocelot_skb, head);
>
> @@ -2510,6 +2520,7 @@ void ocelot_deinit(struct ocelot *ocelot)
>                         dev_kfree_skb_any(entry->skb);
>                         kfree(entry);
>                 }
> +               spin_unlock_irqrestore(&port->skbs_lock, flags);
>         }
>  }
>  EXPORT_SYMBOL(ocelot_deinit);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index e1108a5..7973458 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -426,6 +426,8 @@ struct ocelot_port {
>
>         u8                              ptp_cmd;
>         struct list_head                skbs;
> +       /* Protects the skbs list */
> +       spinlock_t                      skbs_lock;
>         u8                              ts_id;
>  };
>
> --
> 2.7.4
>

Regards,
-Vladimir