On Tue, Oct 17, 2023 at 08:11:28PM +0000, Justin Stitt wrote:
> Let's move away from using strncpy and instead favor a less ambiguous
> and more robust interface.
>
> For ifp->ndev->name, we expect ifp->ndev->name to be NUL-terminated based
> on its use in format strings within core.c:
> 67 | char *brcmf_ifname(struct brcmf_if *ifp)
> 68 | {
> 69 | if (!ifp)
> 70 | return "<if_null>";
> 71 |
> 72 | if (ifp->ndev)
> 73 | return ifp->ndev->name;
> 74 |
> 75 | return "<if_none>";
> 76 | }
> ...
> 288 | static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
> 289 | struct net_device *ndev) {
> ...
> 330 | brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
> 331 | brcmf_ifname(ifp), head_delta);
> ...
> 336 | bphy_err(drvr, "%s: failed to expand headroom\n",
> 337 | brcmf_ifname(ifp));
>
> For di->name, we expect di->name to be NUL-terminated based on its usage
> with format strings:
> | brcms_dbg_dma(di->core,
> | "%s: DMA64 tx doesn't have AE set\n",
> | di->name);
>
> Looking at its allocation we can see that it is already zero-allocated
> which means NUL-padding is not required:
> | di = kzalloc(sizeof(struct dma_info), GFP_ATOMIC);
>
> For wlc->modulecb[i].name, we expect each name in wlc->modulecb to be
> NUL-terminated based on their usage with strcmp():
> | if (!strcmp(wlc->modulecb[i].name, name) &&
>
> NUL-padding is not required as wlc is zero-allocated in:
> brcms_c_attach_malloc() ->
> | wlc = kzalloc(sizeof(struct brcms_c_info), GFP_ATOMIC);
>
> For all these cases, a suitable replacement is `strscpy` due to the fact
> that it guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
>
> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
Good; this looks like standard direct replacements.
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
--
Kees Cook
