netdev - Re: [PATCH v4 net] ps3/gelic: Fix SKB allocation

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: <0b649004-4465-404f-b873-1013bb03a42d@infradead.org>
Date: Fri, 16 Feb 2024 18:37:21 +0900
From: Geoff Levand <geoff@...radead.org>
To: Paolo Abeni <pabeni@...hat.com>, sambat goson <sombat3960@...il.com>,
 Christophe Leroy <christophe.leroy@...roup.eu>,
 "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v4 net] ps3/gelic: Fix SKB allocation

On 2/13/24 21:07, Paolo Abeni wrote:
> On Sat, 2024-02-10 at 17:15 +0900, Geoff Levand wrote:
>> Commit 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures") of
>> 6.8-rc1 did not allocate a network SKB for the gelic_descr, resulting in a
>> kernel panic when the SKB variable (struct gelic_descr.skb) was accessed.
>>
>> This fix changes the way the napi buffer and corresponding SKB are
>> allocated and managed.
> 
> I think this is not what Jakub asked on v3.
> 
> Isn't something alike the following enough to fix the NULL ptr deref?
> 
> Thanks,
> 
> Paolo
> ---
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index d5b75af163d3..51ee6075653f 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -395,7 +395,6 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>         descr->hw_regs.data_error = 0;
>         descr->hw_regs.payload.dev_addr = 0;
>         descr->hw_regs.payload.size = 0;
> -       descr->skb = NULL;

The reason we set the SKB pointer to NULL here is so we can
detect if an SKB has been allocated or not.  If the SKB pointer
is not NULL, then we delete it.

If we just let the SKB pointer be some random value then later
we will try to delete some random address.

-Geoff