linux-kernel - Re: [PATCH 1/2] staging: gpib: Fix NULL pointer dereference in detach

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: <d51c1d1a-f063-4971-b30e-651cd535cf89@stanley.mountain>
Date: Mon, 20 Jan 2025 09:42:04 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Dave Penkler <dpenkler@...il.com>
Cc: gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] staging: gpib: Fix NULL pointer dereference in detach

On Sat, Jan 18, 2025 at 03:50:45PM +0100, Dave Penkler wrote:
> When the detach function is called after a failed attach
> the usb_dev initialization can cause a NULL pointer
> dereference. This happens when the usb device is not found
> in the attach procedure.
> 

What you're describing is true.  The ibonline() calls ->detach if
the ->attach() function fails:

drivers/staging/gpib/agilent_82357a/agilent_82357a.c
   230          retval = board->interface->attach(board, &board->config);
                                           ^^^^^^
   231          if (retval < 0) {
   232                  board->interface->detach(board);
                                          ^^^^^^
   233                  pr_err("gpib: interface attach failed\n");
   234                  return retval;
   235          }

But this is a class of bugs which is very typical when we free things
that are not allocated.  We would normally say, "don't detach things which
are not attached".  I would be surprised if this is the last bug caused by
this particular call to ->detach().

In fact, I notice that agilent_82357a_setup_urbs() has a
kfree(a_priv->interrupt_buffer) so that's a double free bug.  It's better
to have a system so that everyone knows exactly where to put the kfree().
Fortunately #SelfPromotion I have described such a system in my blog.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter