| Message ID | 20240410134858.6313-1-jlee@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | aoe: fix the potential use-after-free problem in more places | expand |
Hi Justin, On Wed, Apr 10, 2024 at 09:48:58PM +0800, Lee, Chun-Yi wrote: > From: Chun-Yi Lee <jlee@suse.com> > > For fixing CVE-2023-6270, f98364e92662 patch moved dev_put() from > aoecmd_cfg_pkts() to tx() to avoid that the tx() runs into use-after-free. > > But Nicolai Stange found more places in aoe have potential use-after-free > problem with tx(). e.g. revalidate(), aoecmd_ata_rw(), resend(), probe() > and aoecmd_cfg_rsp(). Those functions also use aoenet_xmit() to push > packet to tx queue. So they should also use dev_hold() to increase the > refcnt of skb->dev. > > This patch adds dev_hold() to those functions and also uses dev_put() > when the skb_clone() returns NULL. > > Link: https://nvd.nist.gov/vuln/detail/CVE-2023-6270 > Fixes: f98364e92662 ("aoe: fix the potential use-after-free problem in > aoecmd_cfg_pkts") > Reported-by: Nicolai Stange <nstange@suse.com> > Signed-off-by: Chun-Yi Lee <jlee@suse.com> Do you have any suggestion for this patch? or I missed anything? Thanks a lot! Joey Lee > --- > drivers/block/aoe/aoecmd.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c > index cc9077b588d7..fcedbad8e3be 100644 > --- a/drivers/block/aoe/aoecmd.c > +++ b/drivers/block/aoe/aoecmd.c > @@ -361,6 +361,7 @@ ata_rw_frameinit(struct frame *f) > } > > ah->cmdstat = ATA_CMD_PIO_READ | writebit | extbit; > + dev_hold(t->ifp->nd); > skb->dev = t->ifp->nd; > } > > @@ -401,7 +402,8 @@ aoecmd_ata_rw(struct aoedev *d) > __skb_queue_head_init(&queue); > __skb_queue_tail(&queue, skb); > aoenet_xmit(&queue); > - } > + } else > + dev_put(f->t->ifp->nd); > return 1; > } > > @@ -483,10 +485,13 @@ resend(struct aoedev *d, struct frame *f) > memcpy(h->dst, t->addr, sizeof h->dst); > memcpy(h->src, t->ifp->nd->dev_addr, sizeof h->src); > > + dev_hold(t->ifp->nd); > skb->dev = t->ifp->nd; > skb = skb_clone(skb, GFP_ATOMIC); > - if (skb == NULL) > + if (skb == NULL) { > + dev_put(t->ifp->nd); > return; > + } > f->sent = ktime_get(); > __skb_queue_head_init(&queue); > __skb_queue_tail(&queue, skb); > @@ -617,7 +622,8 @@ probe(struct aoetgt *t) > __skb_queue_head_init(&queue); > __skb_queue_tail(&queue, skb); > aoenet_xmit(&queue); > - } > + } else > + dev_put(f->t->ifp->nd); > } > > static long > @@ -1395,6 +1401,7 @@ aoecmd_ata_id(struct aoedev *d) > ah->cmdstat = ATA_CMD_ID_ATA; > ah->lba3 = 0xa0; > > + dev_hold(t->ifp->nd); > skb->dev = t->ifp->nd; > > d->rttavg = RTTAVG_INIT; > @@ -1404,6 +1411,8 @@ aoecmd_ata_id(struct aoedev *d) > skb = skb_clone(skb, GFP_ATOMIC); > if (skb) > f->sent = ktime_get(); > + else > + dev_put(t->ifp->nd); > > return skb; > } > -- > 2.35.3
> For fixing CVE-2023-6270, f98364e92662 patch moved dev_put() from … Please add a subject for the mentioned commit hash. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc6#n99 > This patch adds dev_hold() to those functions and also uses dev_put() > when the skb_clone() returns NULL. Please improve this change description with a corresponding imperative wording. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc6#n94 … > Fixes: f98364e92662 ("aoe: fix the potential use-after-free problem in > aoecmd_cfg_pkts") I suggest to omit a line break for this tag. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc6#n145 … > +++ b/drivers/block/aoe/aoecmd.c … > @@ -401,7 +402,8 @@ aoecmd_ata_rw(struct aoedev *d) > __skb_queue_head_init(&queue); > __skb_queue_tail(&queue, skb); > aoenet_xmit(&queue); > - } > + } else > + dev_put(f->t->ifp->nd); > return 1; > } > … > @@ -617,7 +622,8 @@ probe(struct aoetgt *t) > __skb_queue_head_init(&queue); > __skb_queue_tail(&queue, skb); > aoenet_xmit(&queue); > - } > + } else > + dev_put(f->t->ifp->nd); > } > > static long … Should curly brackets be used for both if branches in these function implementations? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.9-rc6#n213 Regards, Markus
Hi Markus, Thanks for your review! I will send v2 patch. Joey Lee On Tue, Apr 30, 2024 at 06:16:00PM +0200, Markus Elfring wrote: > > For fixing CVE-2023-6270, f98364e92662 patch moved dev_put() from > … > > Please add a subject for the mentioned commit hash. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc6#n99 > > > > This patch adds dev_hold() to those functions and also uses dev_put() > > when the skb_clone() returns NULL. > > Please improve this change description with a corresponding imperative wording. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc6#n94 > > > … > > Fixes: f98364e92662 ("aoe: fix the potential use-after-free problem in > > aoecmd_cfg_pkts") > > I suggest to omit a line break for this tag. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc6#n145 > > > … > > +++ b/drivers/block/aoe/aoecmd.c > … > > @@ -401,7 +402,8 @@ aoecmd_ata_rw(struct aoedev *d) > > __skb_queue_head_init(&queue); > > __skb_queue_tail(&queue, skb); > > aoenet_xmit(&queue); > > - } > > + } else > > + dev_put(f->t->ifp->nd); > > return 1; > > } > > > … > > @@ -617,7 +622,8 @@ probe(struct aoetgt *t) > > __skb_queue_head_init(&queue); > > __skb_queue_tail(&queue, skb); > > aoenet_xmit(&queue); > > - } > > + } else > > + dev_put(f->t->ifp->nd); > > } > > > > static long > … > > Should curly brackets be used for both if branches in these function implementations? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.9-rc6#n213 > > Regards, > Markus
diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c index cc9077b588d7..fcedbad8e3be 100644 --- a/drivers/block/aoe/aoecmd.c +++ b/drivers/block/aoe/aoecmd.c @@ -361,6 +361,7 @@ ata_rw_frameinit(struct frame *f) } ah->cmdstat = ATA_CMD_PIO_READ | writebit | extbit; + dev_hold(t->ifp->nd); skb->dev = t->ifp->nd; } @@ -401,7 +402,8 @@ aoecmd_ata_rw(struct aoedev *d) __skb_queue_head_init(&queue); __skb_queue_tail(&queue, skb); aoenet_xmit(&queue); - } + } else + dev_put(f->t->ifp->nd); return 1; } @@ -483,10 +485,13 @@ resend(struct aoedev *d, struct frame *f) memcpy(h->dst, t->addr, sizeof h->dst); memcpy(h->src, t->ifp->nd->dev_addr, sizeof h->src); + dev_hold(t->ifp->nd); skb->dev = t->ifp->nd; skb = skb_clone(skb, GFP_ATOMIC); - if (skb == NULL) + if (skb == NULL) { + dev_put(t->ifp->nd); return; + } f->sent = ktime_get(); __skb_queue_head_init(&queue); __skb_queue_tail(&queue, skb); @@ -617,7 +622,8 @@ probe(struct aoetgt *t) __skb_queue_head_init(&queue); __skb_queue_tail(&queue, skb); aoenet_xmit(&queue); - } + } else + dev_put(f->t->ifp->nd); } static long @@ -1395,6 +1401,7 @@ aoecmd_ata_id(struct aoedev *d) ah->cmdstat = ATA_CMD_ID_ATA; ah->lba3 = 0xa0; + dev_hold(t->ifp->nd); skb->dev = t->ifp->nd; d->rttavg = RTTAVG_INIT; @@ -1404,6 +1411,8 @@ aoecmd_ata_id(struct aoedev *d) skb = skb_clone(skb, GFP_ATOMIC); if (skb) f->sent = ktime_get(); + else + dev_put(t->ifp->nd); return skb; }