[v1] char: lp: Fix NULL pointer dereference of cad

[PATCH] char: lp: Fix NULL pointer dereference of cad

Posted by linan666@huaweicloud.com 1 month, 3 weeks ago

From: Li Nan <linan122@huawei.com>

NULL pointer dereference occurs when accessing 'port->physport->cad'
as below:

  KASAN: null-ptr-deref in range [0x00000000000003f0-0x00000000000003f7]
  RIP: 0010:parport_wait_peripheral+0x130/0x4b0
  Call Trace:
   parport_ieee1284_write_compat+0x306/0xb70
   ? __pfx_parport_ieee1284_write_compat+0x10/0x10
   parport_write+0x1d6/0x660
   lp_write+0x43e/0xbc0
   ? __pfx_lp_write+0x10/0x10
   vfs_write+0x21c/0x960
   ksys_write+0x12e/0x260
   ? __pfx_ksys_write+0x10/0x10
   ? __audit_syscall_entry+0x39e/0x510
   do_syscall_64+0x59/0x110
   entry_SYSCALL_64_after_hwframe+0x78/0xe2

The root cause is other processes may set 'port->cad' to NULL during
lp_write() operations. Process flow:

  T1					T2
  lp_write
   lock port_mutex *
   lp_claim_parport_or_block
    parport_claim
     port->cad = dev;
   parport_write
    parport_ieee1284_write_compat
					lp_do_ioctl
					 lp_reset
					  lp_release_parport
					   parport_release
					    port->cad = NULL;
     parport_wait_peripheral
      port->physport->cad->timeout
		       |
		      NULL

Fix this issue by adding 'port_mutex' protection. Like read/write and
ioctl LPGETSTATUS, use this lock to protect port access and modification
to prevent concurrency problems.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Li Nan <linan122@huawei.com>
---
 drivers/char/lp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/char/lp.c b/drivers/char/lp.c
index 24417a00dfe9..7e842dfc7722 100644
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
@@ -520,9 +520,14 @@ static int lp_open(struct inode *inode, struct file *file)
 	   should most likely only ever be used by the tunelp application. */
 	if ((LP_F(minor) & LP_ABORTOPEN) && !(file->f_flags & O_NONBLOCK)) {
 		int status;
+		if (mutex_lock_interruptible(&lp_table[minor].port_mutex)) {
+			ret = -EINTR;
+			goto out;
+		}
 		lp_claim_parport_or_block(&lp_table[minor]);
 		status = r_str(minor);
 		lp_release_parport(&lp_table[minor]);
+		mutex_unlock(&lp_table[minor].port_mutex);
 		if (status & LP_POUTPA) {
 			printk(KERN_INFO "lp%d out of paper\n", minor);
 			LP_F(minor) &= ~LP_BUSY;
@@ -547,6 +552,10 @@ static int lp_open(struct inode *inode, struct file *file)
 		goto out;
 	}
 	/* Determine if the peripheral supports ECP mode */
+	if (mutex_lock_interruptible(&lp_table[minor].port_mutex)) {
+		ret = -EINTR;
+		goto out;
+	}
 	lp_claim_parport_or_block(&lp_table[minor]);
 	if ((lp_table[minor].dev->port->modes & PARPORT_MODE_ECP) &&
 	     !parport_negotiate(lp_table[minor].dev->port,
@@ -559,6 +568,7 @@ static int lp_open(struct inode *inode, struct file *file)
 	/* Leave peripheral in compatibility mode */
 	parport_negotiate(lp_table[minor].dev->port, IEEE1284_MODE_COMPAT);
 	lp_release_parport(&lp_table[minor]);
+	mutex_unlock(&lp_table[minor].port_mutex);
 	lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
 out:
 	mutex_unlock(&lp_mutex);
@@ -569,10 +579,13 @@ static int lp_release(struct inode *inode, struct file *file)
 {
 	unsigned int minor = iminor(inode);
 
+	if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
+		return -EINTR;
 	lp_claim_parport_or_block(&lp_table[minor]);
 	parport_negotiate(lp_table[minor].dev->port, IEEE1284_MODE_COMPAT);
 	lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
 	lp_release_parport(&lp_table[minor]);
+	mutex_unlock(&lp_table[minor].port_mutex);
 	kfree(lp_table[minor].lp_buffer);
 	lp_table[minor].lp_buffer = NULL;
 	LP_F(minor) &= ~LP_BUSY;
@@ -641,7 +654,10 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
 				return -EFAULT;
 			break;
 		case LPRESET:
+			if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
+				return -EINTR;
 			lp_reset(minor);
+			mutex_unlock(&lp_table[minor].port_mutex);
 			break;
 #ifdef LP_STATS
 		case LPGETSTATS:
-- 
2.39.2

Re: [PATCH] char: lp: Fix NULL pointer dereference of cad

Posted by Li Nan 1 month, 1 week ago

Friendly ping...

在 2025/12/18 22:20, linan666@huaweicloud.com 写道:
> From: Li Nan <linan122@huawei.com>
> 
> NULL pointer dereference occurs when accessing 'port->physport->cad'
> as below:
> 
>    KASAN: null-ptr-deref in range [0x00000000000003f0-0x00000000000003f7]
>    RIP: 0010:parport_wait_peripheral+0x130/0x4b0
>    Call Trace:
>     parport_ieee1284_write_compat+0x306/0xb70
>     ? __pfx_parport_ieee1284_write_compat+0x10/0x10
>     parport_write+0x1d6/0x660
>     lp_write+0x43e/0xbc0
>     ? __pfx_lp_write+0x10/0x10
>     vfs_write+0x21c/0x960
>     ksys_write+0x12e/0x260
>     ? __pfx_ksys_write+0x10/0x10
>     ? __audit_syscall_entry+0x39e/0x510
>     do_syscall_64+0x59/0x110
>     entry_SYSCALL_64_after_hwframe+0x78/0xe2
> 
> The root cause is other processes may set 'port->cad' to NULL during
> lp_write() operations. Process flow:
> 
>    T1					T2
>    lp_write
>     lock port_mutex *
>     lp_claim_parport_or_block
>      parport_claim
>       port->cad = dev;
>     parport_write
>      parport_ieee1284_write_compat
> 					lp_do_ioctl
> 					 lp_reset
> 					  lp_release_parport
> 					   parport_release
> 					    port->cad = NULL;
>       parport_wait_peripheral
>        port->physport->cad->timeout
> 		       |
> 		      NULL
> 
> Fix this issue by adding 'port_mutex' protection. Like read/write and
> ioctl LPGETSTATUS, use this lock to protect port access and modification
> to prevent concurrency problems.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>   drivers/char/lp.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/char/lp.c b/drivers/char/lp.c
> index 24417a00dfe9..7e842dfc7722 100644
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> @@ -520,9 +520,14 @@ static int lp_open(struct inode *inode, struct file *file)
>   	   should most likely only ever be used by the tunelp application. */
>   	if ((LP_F(minor) & LP_ABORTOPEN) && !(file->f_flags & O_NONBLOCK)) {
>   		int status;
> +		if (mutex_lock_interruptible(&lp_table[minor].port_mutex)) {
> +			ret = -EINTR;
> +			goto out;
> +		}
>   		lp_claim_parport_or_block(&lp_table[minor]);
>   		status = r_str(minor);
>   		lp_release_parport(&lp_table[minor]);
> +		mutex_unlock(&lp_table[minor].port_mutex);
>   		if (status & LP_POUTPA) {
>   			printk(KERN_INFO "lp%d out of paper\n", minor);
>   			LP_F(minor) &= ~LP_BUSY;
> @@ -547,6 +552,10 @@ static int lp_open(struct inode *inode, struct file *file)
>   		goto out;
>   	}
>   	/* Determine if the peripheral supports ECP mode */
> +	if (mutex_lock_interruptible(&lp_table[minor].port_mutex)) {
> +		ret = -EINTR;
> +		goto out;
> +	}
>   	lp_claim_parport_or_block(&lp_table[minor]);
>   	if ((lp_table[minor].dev->port->modes & PARPORT_MODE_ECP) &&
>   	     !parport_negotiate(lp_table[minor].dev->port,
> @@ -559,6 +568,7 @@ static int lp_open(struct inode *inode, struct file *file)
>   	/* Leave peripheral in compatibility mode */
>   	parport_negotiate(lp_table[minor].dev->port, IEEE1284_MODE_COMPAT);
>   	lp_release_parport(&lp_table[minor]);
> +	mutex_unlock(&lp_table[minor].port_mutex);
>   	lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
>   out:
>   	mutex_unlock(&lp_mutex);
> @@ -569,10 +579,13 @@ static int lp_release(struct inode *inode, struct file *file)
>   {
>   	unsigned int minor = iminor(inode);
>   
> +	if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
> +		return -EINTR;
>   	lp_claim_parport_or_block(&lp_table[minor]);
>   	parport_negotiate(lp_table[minor].dev->port, IEEE1284_MODE_COMPAT);
>   	lp_table[minor].current_mode = IEEE1284_MODE_COMPAT;
>   	lp_release_parport(&lp_table[minor]);
> +	mutex_unlock(&lp_table[minor].port_mutex);
>   	kfree(lp_table[minor].lp_buffer);
>   	lp_table[minor].lp_buffer = NULL;
>   	LP_F(minor) &= ~LP_BUSY;
> @@ -641,7 +654,10 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd,
>   				return -EFAULT;
>   			break;
>   		case LPRESET:
> +			if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
> +				return -EINTR;
>   			lp_reset(minor);
> +			mutex_unlock(&lp_table[minor].port_mutex);
>   			break;
>   #ifdef LP_STATS
>   		case LPGETSTATS:

-- 
Thanks,
Nan

Re: [PATCH] char: lp: Fix NULL pointer dereference of cad

Posted by Al Viro 1 month, 1 week ago

On Tue, Dec 30, 2025 at 09:51:43AM +0800, Li Nan wrote:
> Friendly ping...

> > @@ -569,10 +579,13 @@ static int lp_release(struct inode *inode, struct file *file)
> >   {
> >   	unsigned int minor = iminor(inode);
> > +	if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
> > +		return -EINTR;

->release() return value is never checked, simply because there is nothing
to do with it.  It will *not* leave file opened - it will simply leak,
with no way to recover from that.

If you need to report some errors on close, do that in ->flush().
If you ever see ->release() returning a non-zero value, you are very
likely looking at deeply confused code.

Don't do that.  ->release() can't fail, period.  It should've been
void (*release)(struct file *), but for historical reasons it returns
int and there are too many instances to change that.

Re: [PATCH] char: lp: Fix NULL pointer dereference of cad

Posted by Li Nan 1 month, 1 week ago


在 2025/12/30 10:10, Al Viro 写道:
> On Tue, Dec 30, 2025 at 09:51:43AM +0800, Li Nan wrote:
>> Friendly ping...
> 
>>> @@ -569,10 +579,13 @@ static int lp_release(struct inode *inode, struct file *file)
>>>    {
>>>    	unsigned int minor = iminor(inode);
>>> +	if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
>>> +		return -EINTR;
> 
> ->release() return value is never checked, simply because there is nothing
> to do with it.  It will *not* leave file opened - it will simply leak,
> with no way to recover from that.
> 
> If you need to report some errors on close, do that in ->flush().
> If you ever see ->release() returning a non-zero value, you are very
> likely looking at deeply confused code.
> 
> Don't do that.  ->release() can't fail, period.  It should've been
> void (*release)(struct file *), but for historical reasons it returns
> int and there are too many instances to change that.

Thank you for your patient explanation.

Would it be acceptable to switch to mutex_lock() here? Looking at the code
and historical changes, I don't see a compelling reason for the interruptible
function here.

-- 
Thanks,
Nan

Re: [PATCH] char: lp: Fix NULL pointer dereference of cad

Posted by Greg KH 3 weeks, 2 days ago

On Tue, Dec 30, 2025 at 10:52:02AM +0800, Li Nan wrote:
> 
> 
> 在 2025/12/30 10:10, Al Viro 写道:
> > On Tue, Dec 30, 2025 at 09:51:43AM +0800, Li Nan wrote:
> > > Friendly ping...
> > 
> > > > @@ -569,10 +579,13 @@ static int lp_release(struct inode *inode, struct file *file)
> > > >    {
> > > >    	unsigned int minor = iminor(inode);
> > > > +	if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
> > > > +		return -EINTR;
> > 
> > ->release() return value is never checked, simply because there is nothing
> > to do with it.  It will *not* leave file opened - it will simply leak,
> > with no way to recover from that.
> > 
> > If you need to report some errors on close, do that in ->flush().
> > If you ever see ->release() returning a non-zero value, you are very
> > likely looking at deeply confused code.
> > 
> > Don't do that.  ->release() can't fail, period.  It should've been
> > void (*release)(struct file *), but for historical reasons it returns
> > int and there are too many instances to change that.
> 
> Thank you for your patient explanation.
> 
> Would it be acceptable to switch to mutex_lock() here? Looking at the code
> and historical changes, I don't see a compelling reason for the interruptible
> function here.

release should not stall forever like that, so be careful.

Also, do you really have this hardware to test this with?  I'm sure
there are loads of other cleanups / fixes needed in this driver that no
one has done just because they don't have this hardware anymore.
Getting a new maintainer for it would be great.

thanks,

greg k-h

Re: [PATCH] char: lp: Fix NULL pointer dereference of cad

Posted by Li Nan 2 weeks, 6 days ago


在 2026/1/16 22:38, Greg KH 写道:
> On Tue, Dec 30, 2025 at 10:52:02AM +0800, Li Nan wrote:
>>
>>
>> 在 2025/12/30 10:10, Al Viro 写道:
>>> On Tue, Dec 30, 2025 at 09:51:43AM +0800, Li Nan wrote:
>>>> Friendly ping...
>>>
>>>>> @@ -569,10 +579,13 @@ static int lp_release(struct inode *inode, struct file *file)
>>>>>     {
>>>>>     	unsigned int minor = iminor(inode);
>>>>> +	if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
>>>>> +		return -EINTR;
>>>
>>> ->release() return value is never checked, simply because there is nothing
>>> to do with it.  It will *not* leave file opened - it will simply leak,
>>> with no way to recover from that.
>>>
>>> If you need to report some errors on close, do that in ->flush().
>>> If you ever see ->release() returning a non-zero value, you are very
>>> likely looking at deeply confused code.
>>>
>>> Don't do that.  ->release() can't fail, period.  It should've been
>>> void (*release)(struct file *), but for historical reasons it returns
>>> int and there are too many instances to change that.
>>
>> Thank you for your patient explanation.
>>
>> Would it be acceptable to switch to mutex_lock() here? Looking at the code
>> and historical changes, I don't see a compelling reason for the interruptible
>> function here.
> 
> release should not stall forever like that, so be careful.
> 
> Also, do you really have this hardware to test this with?  I'm sure

Thank you for your reply.

Yes, I have the actual hardware for testing. I originally reproduced the
issue in QEMU and will run more comprehensive tests on the real device.

> there are loads of other cleanups / fixes needed in this driver that no
> one has done just because they don't have this hardware anymore.
> Getting a new maintainer for it would be great.
> 

I agree the driver needs further cleanups and fixes. I'm happy to help with
testing and maintenance, and I'm willing to take on the maintainer role for
this driver going forward

> thanks,
> 
> greg k-h

-- 
Thanks,
Nan