netdev - Re: stmmac smatch error rx_queue

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] [day] [month] [year] [list]

Date:   Mon, 22 Jan 2018 18:31:40 +0100
From:   Niklas Cassel <niklas.cassel@...s.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     peppe.cavallaro@...com, alexandre.torgue@...com,
        Joao.Pinto@...opsys.com, netdev@...r.kernel.org
Subject: Re: stmmac smatch error rx_queue_routing

On Mon, Jan 22, 2018 at 04:55:10PM +0000, Jose Abreu wrote:
> Hi Niklas,

Hello Jose, and thanks for your reply.

> > Looking at the code raises some questions:
> >
> >
> > static void dwmac4_tx_queue_routing(struct mac_device_info *hw,
> >                                     u8 packet, u32 queue)
> > {
> >         ...
> >         static const struct stmmac_rx_routing route_possibilities[] = {
> >                 { GMAC_RXQCTRL_AVCPQ_MASK, GMAC_RXQCTRL_AVCPQ_SHIFT },
> >                 { GMAC_RXQCTRL_PTPQ_MASK, GMAC_RXQCTRL_PTPQ_SHIFT },
> >                 { GMAC_RXQCTRL_DCBCPQ_MASK, GMAC_RXQCTRL_DCBCPQ_SHIFT },
> >                 { GMAC_RXQCTRL_UPQ_MASK, GMAC_RXQCTRL_UPQ_SHIFT },
> >                 { GMAC_RXQCTRL_MCBCQ_MASK, GMAC_RXQCTRL_MCBCQ_SHIFT },
> >         };
> >
> >         value = readl(ioaddr + GMAC_RXQ_CTRL1);
> >
> >         /* routing configuration */
> >         value &= ~route_possibilities[packet - 1].reg_mask;
> >         value |= (queue << route_possibilities[packet-1].reg_shift) &
> >                  route_possibilities[packet - 1].reg_mask;
> >
> >
> > Calling the function with e.g. packet == 0 will lead to interesting stuff,
> > so the smatch warning is absolutely warranted.
> 
> Notice that stmmac_mac_config_rx_queues_routing() checks for
> packet == 0x0 and if its true then should never call
> rx_queue_routing, check [1] ...

It would probably be better if the check was in the function instead,
that way we don't have to hope that someone has audited all the places
where the function is called. Also, someone might add a call to that
function that doesn't do an explicit check before calling, and we
probably shouldn't overwrite random memory just because of that.

I also don't see any check that packet <= 6, anywhere,
which would also cause us to overwrite random memory.

> 
> >
> >
> > Looking where this function is used:
> >
> > static const struct stmmac_ops dwmac4_ops = {
> >         ...
> >         .rx_queue_routing = dwmac4_tx_queue_routing,
> >
> > Mixing rx and tx.. is this really correct?
> 
> It should be dwmac4_rx_queue_routing.

Ok. Feel free to submit a patch with my Reported-by if you wish.

> 
> >
> >
> >
> > Looking where the rx_queue_routing function is used:
> > git grep rx_queue_routing
> > stmmac_main.c:  if (rx_queues_count > 1 && priv->hw->mac->rx_queue_routing)
> >
> > it is just referenced in a single place, and we only check if function is
> > non-NULL, we never even call the function, so right now it is just unused
> > code.
> 
> [1] Another typo. You can see that in function
> stmmac_mac_config_rx_queues_routing() we are calling
> rx_queue_prio instead of rx_queue_routing ...

Ah, I see. Feel free to submit a patch that calls the correct function,
with my Reported-by, if you wish.


Regards,
Niklas