stmmac smatch error rx_queue_routing

stmmac smatch error rx_queue_routing

[Niklas Cassel]

Hello stmmac peeps,

I found this smatch error:

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:132 dwmac4_tx_queue_routing() error:
  buffer overflow 'route_possibilities' 5 <= 254
drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:133 dwmac4_tx_queue_routing() error:
  buffer overflow 'route_possibilities' 5 <= 254

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.



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?



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.


Regards,
Niklas

Re: stmmac smatch error rx_queue_routing

[Jose Abreu]

Hi Niklas,

On 22-01-2018 16:43, Niklas Cassel wrote:
> Hello stmmac peeps,
>
> I found this smatch error:
>
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:132 dwmac4_tx_queue_routing() error:
>   buffer overflow 'route_possibilities' 5 <= 254
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:133 dwmac4_tx_queue_routing() error:
>   buffer overflow 'route_possibilities' 5 <= 254
>
> 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] ...

>
>
> 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.

>
>
>
> 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 ...

Best Regards,
Jose Miguel Abreu

>
>
> Regards,
> Niklas

Re: stmmac smatch error rx_queue_routing

[Niklas Cassel]

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