stmmac smatch error rx_queue_routing
Subsystems (2)
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 <= 254drivers/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_routingstmmac_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
Hi Niklas,
Quoted 32 lines · Niklas Cassel
Hello stmmac peeps,
I found this smatch error:
drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:132dwmac4_tx_queue_routing()error:
buffer overflow 'route_possibilities' 5 <= 254drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:133dwmac4_tx_queue_routing()error:
buffer overflow 'route_possibilities' 5 <= 254Looking 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;
Quoted 2 lines
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] ...
Quoted 9 lines
Looking where this function is used:
static const struct
stmmac_opsdwmac4_ops= {
...
.rx_queue_routing =dwmac4_tx_queue_routing,Mixing rx and tx.. is this really correct?
It should be dwmac4_rx_queue_routing.
Quoted 10 lines
Looking where the
rx_queue_routingfunction is used:
git greprx_queue_routingstmmac_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
Quoted 4 lines
Regards,
Niklas
Quoted 1 line · Jose Abreu
Hi Niklas,
Hello Jose, and thanks for your reply.
Quoted 23 lines
Quoted 23 lines
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;
Quoted 6 lines
Quoted 2 lines
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 ==0x0and if its true then should never callrx_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.
Quoted 5 lines
Quoted 5 lines
Looking where this function is used:
Quoted 7 lines
Quoted 5 lines
static const struct
stmmac_opsdwmac4_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.
Quoted 8 lines
Quoted 8 lines
Looking where the
rx_queue_routingfunction is used:
git greprx_queue_routingstmmac_main.c: if (rx_queues_count> 1 &&priv->hw->mac->rx_queue_routing)
Quoted 7 lines
Quoted 3 lines
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 callingrx_queue_prioinstead ofrx_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