* [PATCH] char: lp: Fix NULL pointer dereference of cad
@ 2025-12-18 14:20 linan666
2025-12-30 1:51 ` Li Nan
0 siblings, 1 reply; 6+ messages in thread
From: linan666 @ 2025-12-18 14:20 UTC (permalink / raw)
To: arnd, gregkh; +Cc: linux-kernel, linan666, yangerkun, yi.zhang
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] char: lp: Fix NULL pointer dereference of cad
2025-12-18 14:20 [PATCH] char: lp: Fix NULL pointer dereference of cad linan666
@ 2025-12-30 1:51 ` Li Nan
2025-12-30 2:10 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Li Nan @ 2025-12-30 1:51 UTC (permalink / raw)
To: arnd, gregkh; +Cc: linux-kernel, wanghai (M)
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] char: lp: Fix NULL pointer dereference of cad
2025-12-30 1:51 ` Li Nan
@ 2025-12-30 2:10 ` Al Viro
2025-12-30 2:52 ` Li Nan
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2025-12-30 2:10 UTC (permalink / raw)
To: Li Nan; +Cc: arnd, gregkh, linux-kernel, wanghai (M)
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] char: lp: Fix NULL pointer dereference of cad
2025-12-30 2:10 ` Al Viro
@ 2025-12-30 2:52 ` Li Nan
2026-01-16 14:38 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Li Nan @ 2025-12-30 2:52 UTC (permalink / raw)
To: Al Viro; +Cc: arnd, gregkh, linux-kernel, wanghai (M)
在 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] char: lp: Fix NULL pointer dereference of cad
2025-12-30 2:52 ` Li Nan
@ 2026-01-16 14:38 ` Greg KH
2026-01-20 7:59 ` Li Nan
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2026-01-16 14:38 UTC (permalink / raw)
To: Li Nan; +Cc: Al Viro, arnd, linux-kernel, wanghai (M)
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] char: lp: Fix NULL pointer dereference of cad
2026-01-16 14:38 ` Greg KH
@ 2026-01-20 7:59 ` Li Nan
0 siblings, 0 replies; 6+ messages in thread
From: Li Nan @ 2026-01-20 7:59 UTC (permalink / raw)
To: Greg KH
Cc: Al Viro, arnd, linux-kernel, wanghai (M), yi.zhang@huawei.com,
yangerkun@huawei.com
在 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-01-20 7:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-18 14:20 [PATCH] char: lp: Fix NULL pointer dereference of cad linan666
2025-12-30 1:51 ` Li Nan
2025-12-30 2:10 ` Al Viro
2025-12-30 2:52 ` Li Nan
2026-01-16 14:38 ` Greg KH
2026-01-20 7:59 ` Li Nan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox