RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer
> -Original Message-
> From: Tokunori Ikegami [mailto:ikegam...@gmail.com]
> Sent: Thursday, February 28, 2019 10:26 PM
> To: liujian (CE) ; dw...@infradead.org;
> computersforpe...@gmail.com; bbrezil...@kernel.org;
> marek.va...@gmail.com; rich...@nod.at; joakim.tjernl...@infinera.com;
> ikeg...@allied-telesis.co.jp; keesc...@chromium.org; vigne...@ti.com
> Cc: linux-...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer
>
>
>
> > -Original Message-
> > From: linux-mtd [mailto:linux-mtd-boun...@lists.infradead.org] On
> > Behalf Of Liu Jian
> > Sent: Tuesday, February 26, 2019 11:01 PM
> > To: dw...@infradead.org; computersforpe...@gmail.com;
> > bbrezil...@kernel.org; marek.va...@gmail.com; rich...@nod.at;
> > joakim.tjernl...@infinera.com; ikeg...@allied-telesis.co.jp;
> > keesc...@chromium.org; vigne...@ti.com
> > Cc: linux-...@lists.infradead.org; liujia...@huawei.com;
> > linux-kernel@vger.kernel.org
> > Subject: [PATCH v3] cfi: fix deadloop in cfi_cmdset_0002.c
> > do_write_buffer
> >
> > In function do_write_buffer(), in the for loop, there is a case
> > chip_ready() returns 1 while chip_good() returns 0, so it never break
> > the loop.
> > To fix this, chip_good() is enough and it should timeout if it stay
> > bad for a while.
> >
> > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
> > check correct value")
> > Signed-off-by: Yi Huaijie
> > Signed-off-by: Liu Jian
> > Reviewed-by: Tokunori Ikegami
> > ---
> > v2->v3:
> > Follow Vignesh's advice:
> > add one more check for check_good() even when time_after() returns true.
> >
> > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> > b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 72428b6..3da2376 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct
> > map_info *map, struct flchip *chip,
> > continue;
> > }
> >
> > - if (time_after(jiffies, timeo) && !chip_ready(map, adr))
> > + if (time_after(jiffies, timeo) && !chip_good(map, adr,
> > datum))
>
> Just another idea to understand easily.
>
> unsigned long now = jiffies;
>
> if (chip_good(map, adr, datum)) {
> xip_enable(map, chip, adr);
> goto op_done;
> }
>
> if (time_after(now, timeo) {
> break;
> }
>
Thank you~. It is more easier to understand！
If there are no other comments, I will send new patch again ):
Best Regards,
Liujian
> Regards,
> Ikegami
>
> > break;
> >
> > if (chip_good(map, adr, datum)) {
> > --
> > 2.7.4
> >
> >
> > __
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
RE: [PATCH v2] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer
> -Original Message-
> From: Vignesh R [mailto:vigne...@ti.com]
> Sent: Friday, February 22, 2019 1:59 PM
> To: liujian (CE) ; dw...@infradead.org;
> computersforpe...@gmail.com; bbrezil...@kernel.org;
> marek.va...@gmail.com; rich...@nod.at; joakim.tjernl...@infinera.com;
> ikeg...@allied-telesis.co.jp; keesc...@chromium.org
> Cc: linux-...@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer
>
>
>
> On 20/02/19 2:17 AM, Liu Jian wrote:
> > In function do_write_buffer(), in the for loop, there is a case
> > chip_ready() returns 1 while chip_good() returns 0, so it never break
> > the loop.
> > To fix this, chip_good() is enough and it should timeout if it stay
> > bad for a while.
> >
> > Fixes: dfeae1073583 ("mtd: cfi_cmdset_0002: Change write buffer to
> > check correct value")
> > Signed-off-by: Yi Huaijie
> > Signed-off-by: Liu Jian
> > Reviewed-by: Tokunori Ikegami
> > ---
> > v1->v2:
> > change git log, put the Fixes tag on a single line
> >
> > drivers/mtd/chips/cfi_cmdset_0002.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> > b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index 72428b6..818e94b 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -1876,14 +1876,14 @@ static int __xipram do_write_buffer(struct
> map_info *map, struct flchip *chip,
> > continue;
> > }
> >
> > - if (time_after(jiffies, timeo) && !chip_ready(map, adr))
> > - break;
> > -
> > if (chip_good(map, adr, datum)) {
> > xip_enable(map, chip, adr);
> > goto op_done;
> > }
> >
> > + if (time_after(jiffies, timeo))
> > + break;
> > +
>
> It is quite possible that this thread might be pre-empted just after
> chip_good() check but before before time_after(). If the thread, then resumes
> execution after timeo has elasped then, this code will wrongly indicate write
> failure.
>
> To avoid this case, you should add one more check for check_good() even when
> time_after() returns true. Something like:
>
> if (time_after(jiffies, timeo)) {
> if (chip_good(map, adr, datum)) {
> xip_enable(map, chip, adr);
> goto op_done;
> }
> break;
> }
>
So, the patch should like this ?
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
b/drivers/mtd/chips/cfi_cmdset_0002.c
index 72428b6..3da2376 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -1876,7 +1876,7 @@ static int __xipram do_write_buffer(struct map_info *map,
struct flchip *chip,
continue;
}
- if (time_after(jiffies, timeo) && !chip_ready(map, adr))
+ if (time_after(jiffies, timeo) && !chip_good(map, adr, datum))
break;
if (chip_good(map, adr, datum)) {
Any other opinions?
If there are no other comments, I will send a patch again
>
> > /* Latency issues. Drop the lock, wait a while and retry */
> > UDELAY(map, chip, adr, 1);
> > }
> >
>
> --
> Regards
> Vignesh
RE: Re: [PATCH] cfi: fix deadloop in cfi_cmdset_0002.c do_write_buffer
Best Regards, liujian > -Original Message- > From: Tokunori Ikegami [mailto:ikegam...@gmail.com] > Sent: Friday, February 08, 2019 10:24 PM > To: 'Sobon, Przemyslaw' ; 'Boris Brezillon' > > Cc: keesc...@chromium.org; marek.va...@gmail.com; rich...@nod.at; > linux-kernel@vger.kernel.org; joakim.tjernl...@infinera.com; > linux-...@lists.infradead.org; computersforpe...@gmail.com; > dw...@infradead.org; liujian (CE) ; > ikegami...@yahoo.co.jp > Subject: RE: Re: [PATCH] cfi: fix deadloop in cfi_cmdset_0002.c > do_write_buffer > > Hi Przemek-san, > > Thank you so much for your explanation. > > > I have seen a case myself where a value was written, chip changed > > state to "ready" but when I was reading the value was incorrect. > > I also know the similar issues for the both buffer and word write. > Both issues were able to reproduce the write error behavior. > Note: The word write issue is able to reproduce now also. > > Those were resolved by using chip_good() instead to check the state. > > > This can happen as result of intermittent issue with flash. It is hard > > to fall into scenario when testing on limited number of devices but > > with large enough population you can see that. > > If possible I would like to know the issue detail and its cause also. > > > Another situation > > is when a flash chip reaches its maximum number of writes. So for > > example a chip is designed for 100k writes to a page. Once you reach > > that number of writes you can have invalid data written to flash but > > chip itself reports everything was good and switches to "ready" state. > > Yes I see. > > Regards, > Ikegami > > > -Original Message- > > From: linux-mtd [mailto:linux-mtd-boun...@lists.infradead.org] On > > Behalf Of Sobon, Przemyslaw > > Sent: Friday, February 8, 2019 8:51 AM > > To: ikegami...@yahoo.co.jp; Boris Brezillon > > Cc: keesc...@chromium.org; marek.va...@gmail.com; > > ikeg...@allied-telesis.co.jp; rich...@nod.at; > > linux-kernel@vger.kernel.org; joakim.tjernl...@infinera.com; > > linux-...@lists.infradead.org; computersforpe...@gmail.com; > > dw...@infradead.org; Liu Jian > > Subject: RE: Re: [PATCH] cfi: fix deadloop in cfi_cmdset_0002.c > > do_write_buffer > > > > Hi Ikegami, > > > > I have seen a case myself where a value was written, chip changed > > state to "ready" but when I was reading the value was incorrect. > > This can happen as result of intermittent issue with flash. It is hard > > to fall into scenario when testing on limited number of devices but > > with large enough population you can see that. Another situation is > > when a flash chip reaches its maximum number of writes. So for example > > a chip is designed for 100k writes to a page. Once you reach that > > number of writes you can have invalid data written to flash but chip > > itself reports everything was good and switches to "ready" state. > > > > Hope this explanation is clear. Please let me know. > > > > Regards, > > Przemek > > > > > -Original Message- > > > From: ikegami...@yahoo.co.jp > > > Sent: Thursday, February 7, 2019 3:00 PM > > > > > > Hi Przemek-san, > > > > > > Could you please explain the case detail that the value is written > > incorrectly? > > > I think that the value is only written correctly except a bug. > > > > > > Regards, > > > Ikegami > > > > > > --- boris.brezil...@collabora.com wrote --- : > > > > Hi Sobon, > > > > > > > > On Tue, 5 Feb 2019 22:28:44 + > > > > "Sobon, Przemyslaw" wrote: > > > > > > > > > > From: Boris Brezillon > > > > > > Sent: Sunday, February 3, 2019 12:35 AM > > > > > > > +Przemyslaw > > > > > > > > > > > > > > On Fri, 1 Feb 2019 07:30:39 +0800 Liu Jian > > > > > > > wrote: > > > > > > > > > > > > > > > In function do_write_buffer(), in the for loop, there is a > > > > > > > > case > > > > > > > > chip_ready() returns 1 while chip_good() returns 0, so it > > > > > > > > never break the loop. > > > > > > > > To fix this, chip_good() is enough and it should timeout > > > > > > > > if > > it > > > > > > > > stay bad for a while. > > > > > > &g
RE: [PATCH v5] driver: uio: fix possible memory leak in uio_open
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, January 31, 2019 11:36 PM
> To: liujian (CE)
> Cc: xiu...@redhat.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5] driver: uio: fix possible memory leak in uio_open
>
> On Wed, Jan 23, 2019 at 06:38:24AM +0800, Liu Jian wrote:
> > If 'idev->info' is NULL, we need to free 'listener'
> >
> > Fixes: 57c5f4df0a5a ("uio: fix crash after the device is
> > unregistered")
> > Signed-off-by: Liu Jian
> > ---
> > v1->v2:
> > rename the "err_infoopen" to "err_idev_info"
> > v2->3:
> > put the extra info after the "--"
> > v3-v4:
> > add git log
> > v4-v5:
> > correct git log
> >
> > drivers/uio/uio.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index
> > 1313422..b4ae2d9 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -491,18 +491,19 @@ static int uio_open(struct inode *inode, struct
> file *filep)
> > if (!idev->info) {
> > mutex_unlock(>info_lock);
> > ret = -EINVAL;
> > - goto err_alloc_listener;
> > + goto err_idev_info;
> > }
> >
> > if (idev->info && idev->info->open)
> > ret = idev->info->open(idev->info, inode);
> > mutex_unlock(>info_lock);
> > if (ret)
> > - goto err_infoopen;
> > + goto err_idev_info;
> >
> > return 0;
> >
> > -err_infoopen:
> > +err_idev_info:
> > + filep->private_data = NULL;
> > kfree(listener);
> >
> > err_alloc_listener:
> > --
> > 2.7.4
> >
>
> This does not apply to my tree at all :(
I am sorry to have sent so many versions, during which this issue has been
resolved by commit 1e09cdd506c8833a9d52cb61009798660cff4051.
So please ignore this patch , and thank you and Xiubo Li in this patch~
> Please rebase it against the char-misc-next branch of my char-misc.git
> tree and resend.
>
> thanks,
>
> greg k-h
RE: [PATCH v3] driver: uio: fix possible memory leak in uio_open
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, January 08, 2019 12:14 AM
> To: liujian (CE)
> Cc: xiu...@redhat.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] driver: uio: fix possible memory leak in uio_open
>
> On Thu, Jan 03, 2019 at 10:28:22PM +0800, liujian wrote:
> > Fixes: 57c5f4df0a5a ("uio: fix crash after the device is
> > unregistered")
> > Signed-off-by: liujian
>
>
> I can not take patches without any changelog text at all :(
I am sorry, I will resend the patch~
>
> Also, is "liujian" your full name? We need a real and full name for a
> signed-off-by line.
Yes, but as someone do, I may need change 'liujian' to 'Liu Jian'
Thanks
> thanks,
>
> greg k-h
RE: [PATCH v2] driver: uio: fix possible memory leak and use-after-free in __uio_register_device
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, January 08, 2019 12:13 AM
> To: liujian (CE)
> Cc: michal.si...@xilinx.com; hamish.mar...@alliedtelesis.co.nz;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] driver: uio: fix possible memory leak and
> use-after-free
> in __uio_register_device
>
> On Fri, Jan 04, 2019 at 10:19:08PM +0800, liujian wrote:
> > 'idev' is malloced in __uio_register_device() and leak free it before
> > leaving from the uio_get_minor() error handing case, it will cause
> > memory leak.
> >
> > Also, in uio_dev_add_attributes() error handing case, idev is used
> > after device_unregister(), in which 'idev' has been released, touch
> > idev cause use-after-free.
> >
> > Fixes: a93e7b331568 ("uio: Prevent device destruction while fds are
> > open")
> > Fixes: e6789cd3dfb5 ("uio: Simplify uio error path by using devres
> > functions")
> > Signed-off-by: liujian
> > Reviewed-by: Hamish Martin
> > ---
> > v1->v2:
> > change git log and fix code
> >
> > drivers/uio/uio.c | 10 +++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index
> > 1313422..be2a943 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -940,9 +940,12 @@ int __uio_register_device(struct module *owner,
> > atomic_set(>event, 0);
> >
> > ret = uio_get_minor(idev);
> > - if (ret)
> > + if (ret) {
> > + kfree(idev);
> > return ret;
> > + }
> >
> > + device_initialize(>dev);
> > idev->dev.devt = MKDEV(uio_major, idev->minor);
> > idev->dev.class = _class;
> > idev->dev.parent = parent;
> > @@ -953,7 +956,7 @@ int __uio_register_device(struct module *owner,
> > if (ret)
> > goto err_device_create;
> >
> > - ret = device_register(>dev);
> > + ret = device_add(>dev);
> > if (ret)
> > goto err_device_create;
> >
> > @@ -985,9 +988,10 @@ int __uio_register_device(struct module *owner,
> > err_request_irq:
> > uio_dev_del_attributes(idev);
> > err_uio_dev_add_attributes:
> > - device_unregister(>dev);
> > + device_del(>dev);
> > err_device_create:
> > uio_free_minor(idev);
> > + put_device(>dev);
>
> device_del() and then put_device()? I don't think that's a correct error
> cleanup path do you?
>
In err_uio_dev_add_attributes error handling case, device_unregister() will
call put_device(), and idev will be released in put_device()
Then in err_device_create error handling case, uio_free_minor() will trigger
use-after-free
Now, divide device_register() into device_initialize() and device_add(), and
divide device_unregister() into device_del() and put_device().
Call uio_free_minor() and then call put_device() to avoid use-after-free
> Please fix one thing at a time here also, this should be a a patch series,
> right?
>
Yes, I will do, thanks~
Best Regards,
Liujian
> thanks,
>
> greg k-h
[PATCH v2] driver: uio: fix possible memory leak and use-after-free in __uio_register_device
'idev' is malloced in __uio_register_device() and leak free it before
leaving from the uio_get_minor() error handing case, it will cause
memory leak.
Also, in uio_dev_add_attributes() error handing case, idev is used after
device_unregister(), in which 'idev' has been released, touch idev cause
use-after-free.
Fixes: a93e7b331568 ("uio: Prevent device destruction while fds are open")
Fixes: e6789cd3dfb5 ("uio: Simplify uio error path by using devres
functions")
Signed-off-by: liujian
---
v1->v2:
change git log and fix code
drivers/uio/uio.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 1313422..be2a943 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -940,9 +940,12 @@ int __uio_register_device(struct module *owner,
atomic_set(>event, 0);
ret = uio_get_minor(idev);
- if (ret)
+ if (ret) {
+ kfree(idev);
return ret;
+ }
+ device_initialize(>dev);
idev->dev.devt = MKDEV(uio_major, idev->minor);
idev->dev.class = _class;
idev->dev.parent = parent;
@@ -953,7 +956,7 @@ int __uio_register_device(struct module *owner,
if (ret)
goto err_device_create;
- ret = device_register(>dev);
+ ret = device_add(>dev);
if (ret)
goto err_device_create;
@@ -985,9 +988,10 @@ int __uio_register_device(struct module *owner,
err_request_irq:
uio_dev_del_attributes(idev);
err_uio_dev_add_attributes:
- device_unregister(>dev);
+ device_del(>dev);
err_device_create:
uio_free_minor(idev);
+ put_device(>dev);
return ret;
}
EXPORT_SYMBOL_GPL(__uio_register_device);
--
2.7.4
[PATCH v3] driver: uio: fix possible memory leak in uio_open
Fixes: 57c5f4df0a5a ("uio: fix crash after the device is unregistered")
Signed-off-by: liujian
---
v1->v2:
rename the "err_infoopen" to "err_idev_info"
v2->3:
put the extra info after the "--"
drivers/uio/uio.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5c10fc7..aab3520 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -496,18 +496,19 @@ static int uio_open(struct inode *inode, struct file
*filep)
if (!idev->info) {
mutex_unlock(>info_lock);
ret = -EINVAL;
- goto err_alloc_listener;
+ goto err_idev_info;
}
if (idev->info && idev->info->open)
ret = idev->info->open(idev->info, inode);
mutex_unlock(>info_lock);
if (ret)
- goto err_infoopen;
+ goto err_idev_info;
return 0;
-err_infoopen:
+err_idev_info:
+ filep->private_data = NULL;
kfree(listener);
err_alloc_listener:
--
2.7.4
[PATCH v2] driver: uio: fix possible memory leak in uio_open
v1->v2:
rename the "err_infoopen" to "err_idev_info"
Fixes: 57c5f4df0a5a ("uio: fix crash after the device is unregistered")
Signed-off-by: liujian
---
drivers/uio/uio.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5c10fc7..aab3520 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -496,18 +496,19 @@ static int uio_open(struct inode *inode, struct file
*filep)
if (!idev->info) {
mutex_unlock(>info_lock);
ret = -EINVAL;
- goto err_alloc_listener;
+ goto err_idev_info;
}
if (idev->info && idev->info->open)
ret = idev->info->open(idev->info, inode);
mutex_unlock(>info_lock);
if (ret)
- goto err_infoopen;
+ goto err_idev_info;
return 0;
-err_infoopen:
+err_idev_info:
+ filep->private_data = NULL;
kfree(listener);
err_alloc_listener:
--
2.7.4
RE: [PATCH] driver: uio: fix possible memory leak in uio_open
> -Original Message-
> From: linux-kernel-ow...@vger.kernel.org
> [mailto:linux-kernel-ow...@vger.kernel.org] On Behalf Of Xiubo Li
> Sent: Wednesday, January 02, 2019 2:37 PM
> To: liujian (CE)
> Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] driver: uio: fix possible memory leak in uio_open
>
> On 2019/1/3 0:26, liujian wrote:
> > Fixes: 57c5f4df0a5a ("uio: fix crash after the device is
> > unregistered")
> > Signed-off-by: liujian
> > ---
> > drivers/uio/uio.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c index
> > 5c10fc7..bde7d7a 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -496,7 +496,7 @@ static int uio_open(struct inode *inode, struct file
> *filep)
> > if (!idev->info) {
> > mutex_unlock(>info_lock);
> > ret = -EINVAL;
> > - goto err_alloc_listener;
> > + goto err_infoopen;
> > }
> >
> > if (idev->info && idev->info->open) @@ -508,6 +508,7 @@ static int
> > uio_open(struct inode *inode, struct file *filep)
> > return 0;
> >
> > err_infoopen:
>
> Maybe we should rename the "err_infoopen" to something like
> "err_idev_info"...
>
Yes, it's better to change this, I will send v2, thank you~
Best Regards,
liujian
> Thanks.
>
> BRs
>
> > + filep->private_data = NULL;
> > kfree(listener);
> >
> > err_alloc_listener:
>
[PATCH] driver: uio: fix possible memory leak in uio_open
Fixes: 57c5f4df0a5a ("uio: fix crash after the device is unregistered")
Signed-off-by: liujian
---
drivers/uio/uio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5c10fc7..bde7d7a 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -496,7 +496,7 @@ static int uio_open(struct inode *inode, struct file *filep)
if (!idev->info) {
mutex_unlock(>info_lock);
ret = -EINVAL;
- goto err_alloc_listener;
+ goto err_infoopen;
}
if (idev->info && idev->info->open)
@@ -508,6 +508,7 @@ static int uio_open(struct inode *inode, struct file *filep)
return 0;
err_infoopen:
+ filep->private_data = NULL;
kfree(listener);
err_alloc_listener:
--
2.7.4
[PATCH] driver: uio: fix possible memory leak and use-after-free in __uio_register_device
Before device_register, if something goes wrong, we need to manually
free idev.
In the error handling path, after device_unregister, idev maybe have been
released, we should not use it anymore.
Signed-off-by: liujian
---
drivers/uio/uio.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 1313422..5c10fc7 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -413,13 +413,18 @@ static int uio_get_minor(struct uio_device *idev)
return retval;
}
-static void uio_free_minor(struct uio_device *idev)
+static void __uio_free_minor(int id)
{
mutex_lock(_lock);
- idr_remove(_idr, idev->minor);
+ idr_remove(_idr, id);
mutex_unlock(_lock);
}
+static void uio_free_minor(struct uio_device *idev)
+{
+ __uio_free_minor(idev->minor);
+}
+
/**
* uio_event_notify - trigger an interrupt event
* @info: UIO device capabilities
@@ -919,6 +924,7 @@ int __uio_register_device(struct module *owner,
{
struct uio_device *idev;
int ret = 0;
+ int uio_minor;
if (!uio_class_registered)
return -EPROBE_DEFER;
@@ -940,8 +946,12 @@ int __uio_register_device(struct module *owner,
atomic_set(>event, 0);
ret = uio_get_minor(idev);
- if (ret)
+ if (ret) {
+ kfree(idev);
return ret;
+ }
+
+ uio_minor = idev->minor;
idev->dev.devt = MKDEV(uio_major, idev->minor);
idev->dev.class = _class;
@@ -950,8 +960,11 @@ int __uio_register_device(struct module *owner,
dev_set_drvdata(>dev, idev);
ret = dev_set_name(>dev, "uio%d", idev->minor);
- if (ret)
- goto err_device_create;
+ if (ret) {
+ __uio_free_minor(uio_minor);
+ kfree(idev);
+ return ret;
+ }
ret = device_register(>dev);
if (ret)
@@ -987,7 +1000,7 @@ int __uio_register_device(struct module *owner,
err_uio_dev_add_attributes:
device_unregister(>dev);
err_device_create:
- uio_free_minor(idev);
+ __uio_free_minor(uio_minor);
return ret;
}
EXPORT_SYMBOL_GPL(__uio_register_device);
--
2.7.4
RE: [PATCH] driver: input: fix UBSAN warning in input_defuzz_abs_event
Best Regards,
liujian
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, November 13, 2018 3:49 AM
> To: liujian (CE)
> Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] driver: input: fix UBSAN warning in
> input_defuzz_abs_event
>
> Hi,
>
> On Fri, Nov 02, 2018 at 09:48:51PM +0800, liujian wrote:
> > syzkaller triggered a UBCAN warning:
> >
> > [ 196.188950] UBSAN: Undefined behaviour in
> > drivers/input/input.c:62:23 [ 196.188958] signed integer overflow:
> > [ 196.188964] -2147483647 - 104 cannot be represented in type 'int [2]'
> > [ 196.188973] CPU: 7 PID: 4763 Comm: syz-executor Not tainted
> > 4.19.0-514.55.6.9.x86_64+ #7 [ 196.188977] Hardware name: Bochs
> > Bochs, BIOS Bochs 01/01/2011 [ 196.188979] Call Trace:
> > [ 196.189001] dump_stack+0x91/0xeb
> > [ 196.189014] ubsan_epilogue+0x9/0x7c [ 196.189020]
> > handle_overflow+0x1d7/0x22c [ 196.189028] ?
> > __ubsan_handle_negate_overflow+0x18f/0x18f
> > [ 196.189038] ? __mutex_lock+0x213/0x13f0 [ 196.189053] ?
> > drop_futex_key_refs+0xa0/0xa0 [ 196.189070] ?
> > __might_fault+0xef/0x1b0 [ 196.189096]
> > input_handle_event+0xe1b/0x1290 [ 196.189108]
> > input_inject_event+0x1d7/0x27e [ 196.189119]
> evdev_write+0x2cf/0x3f0
> > [ 196.189129] ? evdev_pass_values+0xd40/0xd40 [ 196.189157] ?
> > mark_held_locks+0x160/0x160 [ 196.189171] ?
> __vfs_write+0xe0/0x6c0 [
> > 196.189175] ? evdev_pass_values+0xd40/0xd40 [ 196.189179]
> > __vfs_write+0xe0/0x6c0 [ 196.189186] ? kernel_read+0x130/0x130 [
> > 196.189204] ? _cond_resched+0x15/0x30 [ 196.189214] ?
> > __inode_security_revalidate+0xb8/0xe0
> > [ 196.189222] ? selinux_file_permission+0x354/0x430
> > [ 196.189233] vfs_write+0x160/0x440
> > [ 196.189242] ksys_write+0xc1/0x190
> > [ 196.189248] ? __ia32_sys_read+0xb0/0xb0 [ 196.189259] ?
> > trace_hardirqs_on_thunk+0x1a/0x1c [ 196.189267] ?
> > do_syscall_64+0x22/0x4a0 [ 196.189276] do_syscall_64+0xa5/0x4a0 [
> > 196.189287] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [ 196.189293] RIP: 0033:0x44e7c9
> > [ 196.189299] Code: fc ff 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f
> > 00
> >
> > the syzkaller reproduce script(but can't reproduce it every time):
> >
> > r0 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2,
> > 0x1)
> > write$binfmt_elf64(r0, &(0x7f000240)={{0x7f, 0x45, 0x4c, 0x46,
> > 0x40, 0x2, 0x2, 0x, 0x374c, 0x3, 0x0, 0x8001,
> > 0x103, 0x40, 0x22e, 0x26, 0x1, 0x38, 0x2, 0xa23, 0x1, 0x2},
> > [{0x6474e557, 0x5, 0x6, 0x2, 0x9, 0x9, 0x6c3, 0x1ff}], "", [[], [],
> > [], []]}, 0x478) ioctl$EVIOCGSW(0x, 0x8040451b,
> > &(0x7f40)=""/7)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x1)
> > r1 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2,
> > 0x1)
> > openat$smack_task_current(0xff9c,
> > &(0x7f40)='/proc/self/attr/current\x00', 0x2, 0x0)
> > ioctl$EVIOCSABS0(r1, 0x401845c0, &(0x7f00)={0x4, 0x1,
> 0x4,
> > 0xd1, 0x81, 0x3})
> > eventfd(0x1ff)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x200)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x1) syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2, 0x1)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x1) syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2, 0x1)
> >
> > Typecast int to long to fix the issue.
>
> Does this fix 32-bit platforms where long equals int? BTW, I'd prefer if we
> did
> not have to do 64 bit arithmetic on 32 bit arches here, if possible.
Hi Dmitry,
Thanks for your review, we can typecast it to long long? but if do not 64 bit
arithmetic on 32-bit platforms, how about this?
We can not care about the overflow of addition/subtraction, but we should care
about the return value.?
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaa..954244f 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -59,14 +59,17 @@ static inline int is_event_supported(unsigned int code,
static int input_defuzz_abs_event(int value, int old_val, int fuzz)
{
if (fuzz) {
- if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2)
+ if (value > (long)((unsigned long)old_val - (u
RE: [PATCH] driver: input: fix UBSAN warning in input_defuzz_abs_event
Best Regards,
liujian
> -Original Message-
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Tuesday, November 13, 2018 3:49 AM
> To: liujian (CE)
> Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] driver: input: fix UBSAN warning in
> input_defuzz_abs_event
>
> Hi,
>
> On Fri, Nov 02, 2018 at 09:48:51PM +0800, liujian wrote:
> > syzkaller triggered a UBCAN warning:
> >
> > [ 196.188950] UBSAN: Undefined behaviour in
> > drivers/input/input.c:62:23 [ 196.188958] signed integer overflow:
> > [ 196.188964] -2147483647 - 104 cannot be represented in type 'int [2]'
> > [ 196.188973] CPU: 7 PID: 4763 Comm: syz-executor Not tainted
> > 4.19.0-514.55.6.9.x86_64+ #7 [ 196.188977] Hardware name: Bochs
> > Bochs, BIOS Bochs 01/01/2011 [ 196.188979] Call Trace:
> > [ 196.189001] dump_stack+0x91/0xeb
> > [ 196.189014] ubsan_epilogue+0x9/0x7c [ 196.189020]
> > handle_overflow+0x1d7/0x22c [ 196.189028] ?
> > __ubsan_handle_negate_overflow+0x18f/0x18f
> > [ 196.189038] ? __mutex_lock+0x213/0x13f0 [ 196.189053] ?
> > drop_futex_key_refs+0xa0/0xa0 [ 196.189070] ?
> > __might_fault+0xef/0x1b0 [ 196.189096]
> > input_handle_event+0xe1b/0x1290 [ 196.189108]
> > input_inject_event+0x1d7/0x27e [ 196.189119]
> evdev_write+0x2cf/0x3f0
> > [ 196.189129] ? evdev_pass_values+0xd40/0xd40 [ 196.189157] ?
> > mark_held_locks+0x160/0x160 [ 196.189171] ?
> __vfs_write+0xe0/0x6c0 [
> > 196.189175] ? evdev_pass_values+0xd40/0xd40 [ 196.189179]
> > __vfs_write+0xe0/0x6c0 [ 196.189186] ? kernel_read+0x130/0x130 [
> > 196.189204] ? _cond_resched+0x15/0x30 [ 196.189214] ?
> > __inode_security_revalidate+0xb8/0xe0
> > [ 196.189222] ? selinux_file_permission+0x354/0x430
> > [ 196.189233] vfs_write+0x160/0x440
> > [ 196.189242] ksys_write+0xc1/0x190
> > [ 196.189248] ? __ia32_sys_read+0xb0/0xb0 [ 196.189259] ?
> > trace_hardirqs_on_thunk+0x1a/0x1c [ 196.189267] ?
> > do_syscall_64+0x22/0x4a0 [ 196.189276] do_syscall_64+0xa5/0x4a0 [
> > 196.189287] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > [ 196.189293] RIP: 0033:0x44e7c9
> > [ 196.189299] Code: fc ff 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f
> > 00
> >
> > the syzkaller reproduce script(but can't reproduce it every time):
> >
> > r0 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2,
> > 0x1)
> > write$binfmt_elf64(r0, &(0x7f000240)={{0x7f, 0x45, 0x4c, 0x46,
> > 0x40, 0x2, 0x2, 0x, 0x374c, 0x3, 0x0, 0x8001,
> > 0x103, 0x40, 0x22e, 0x26, 0x1, 0x38, 0x2, 0xa23, 0x1, 0x2},
> > [{0x6474e557, 0x5, 0x6, 0x2, 0x9, 0x9, 0x6c3, 0x1ff}], "", [[], [],
> > [], []]}, 0x478) ioctl$EVIOCGSW(0x, 0x8040451b,
> > &(0x7f40)=""/7)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x1)
> > r1 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2,
> > 0x1)
> > openat$smack_task_current(0xff9c,
> > &(0x7f40)='/proc/self/attr/current\x00', 0x2, 0x0)
> > ioctl$EVIOCSABS0(r1, 0x401845c0, &(0x7f00)={0x4, 0x1,
> 0x4,
> > 0xd1, 0x81, 0x3})
> > eventfd(0x1ff)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x200)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x1) syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2, 0x1)
> > syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
> > 0x1) syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00',
> > 0x2, 0x1)
> >
> > Typecast int to long to fix the issue.
>
> Does this fix 32-bit platforms where long equals int? BTW, I'd prefer if we
> did
> not have to do 64 bit arithmetic on 32 bit arches here, if possible.
Hi Dmitry,
Thanks for your review, we can typecast it to long long? but if do not 64 bit
arithmetic on 32-bit platforms, how about this?
We can not care about the overflow of addition/subtraction, but we should care
about the return value.?
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaa..954244f 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -59,14 +59,17 @@ static inline int is_event_supported(unsigned int code,
static int input_defuzz_abs_event(int value, int old_val, int fuzz)
{
if (fuzz) {
- if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2)
+ if (value > (long)((unsigned long)old_val - (u
[PATCH] driver: input: fix UBSAN warning in input_defuzz_abs_event
syzkaller triggered a UBCAN warning:
[ 196.188950] UBSAN: Undefined behaviour in drivers/input/input.c:62:23
[ 196.188958] signed integer overflow:
[ 196.188964] -2147483647 - 104 cannot be represented in type 'int [2]'
[ 196.188973] CPU: 7 PID: 4763 Comm: syz-executor Not tainted
4.19.0-514.55.6.9.x86_64+ #7
[ 196.188977] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 196.188979] Call Trace:
[ 196.189001] dump_stack+0x91/0xeb
[ 196.189014] ubsan_epilogue+0x9/0x7c
[ 196.189020] handle_overflow+0x1d7/0x22c
[ 196.189028] ? __ubsan_handle_negate_overflow+0x18f/0x18f
[ 196.189038] ? __mutex_lock+0x213/0x13f0
[ 196.189053] ? drop_futex_key_refs+0xa0/0xa0
[ 196.189070] ? __might_fault+0xef/0x1b0
[ 196.189096] input_handle_event+0xe1b/0x1290
[ 196.189108] input_inject_event+0x1d7/0x27e
[ 196.189119] evdev_write+0x2cf/0x3f0
[ 196.189129] ? evdev_pass_values+0xd40/0xd40
[ 196.189157] ? mark_held_locks+0x160/0x160
[ 196.189171] ? __vfs_write+0xe0/0x6c0
[ 196.189175] ? evdev_pass_values+0xd40/0xd40
[ 196.189179] __vfs_write+0xe0/0x6c0
[ 196.189186] ? kernel_read+0x130/0x130
[ 196.189204] ? _cond_resched+0x15/0x30
[ 196.189214] ? __inode_security_revalidate+0xb8/0xe0
[ 196.189222] ? selinux_file_permission+0x354/0x430
[ 196.189233] vfs_write+0x160/0x440
[ 196.189242] ksys_write+0xc1/0x190
[ 196.189248] ? __ia32_sys_read+0xb0/0xb0
[ 196.189259] ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 196.189267] ? do_syscall_64+0x22/0x4a0
[ 196.189276] do_syscall_64+0xa5/0x4a0
[ 196.189287] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 196.189293] RIP: 0033:0x44e7c9
[ 196.189299] Code: fc ff 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f 00
the syzkaller reproduce script(but can't reproduce it every time):
r0 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
0x1)
write$binfmt_elf64(r0, &(0x7f000240)={{0x7f, 0x45, 0x4c, 0x46, 0x40,
0x2, 0x2, 0x, 0x374c, 0x3, 0x0, 0x8001, 0x103,
0x40, 0x22e, 0x26, 0x1, 0x38, 0x2, 0xa23, 0x1, 0x2}, [{0x6474e557, 0x5,
0x6, 0x2, 0x9, 0x9, 0x6c3, 0x1ff}], "", [[], [], [], []]}, 0x478)
ioctl$EVIOCGSW(0x, 0x8040451b, &(0x7f40)=""/7)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
r1 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
0x1)
openat$smack_task_current(0xff9c,
&(0x7f40)='/proc/self/attr/current\x00', 0x2, 0x0)
ioctl$EVIOCSABS0(r1, 0x401845c0, &(0x7f00)={0x4, 0x1, 0x4,
0xd1, 0x81, 0x3})
eventfd(0x1ff)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
0x200)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
Typecast int to long to fix the issue.
Signed-off-by: liujian
---
drivers/input/input.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaa..24615ef 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -59,14 +59,17 @@ static inline int is_event_supported(unsigned int code,
static int input_defuzz_abs_event(int value, int old_val, int fuzz)
{
if (fuzz) {
- if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2)
+ if (value > (long)old_val - fuzz / 2 &&
+ value < (long)old_val + fuzz / 2)
return old_val;
- if (value > old_val - fuzz && value < old_val + fuzz)
- return (old_val * 3 + value) / 4;
+ if (value > (long)old_val - fuzz &&
+ value < (long)old_val + fuzz)
+ return ((long)old_val * 3 + value) / 4;
- if (value > old_val - fuzz * 2 && value < old_val + fuzz * 2)
- return (old_val + value) / 2;
+ if (value > (long)old_val - fuzz * 2 &&
+ value < (long)old_val + fuzz * 2)
+ return ((long)old_val + value) / 2;
}
return value;
--
2.7.4
[PATCH] driver: input: fix UBSAN warning in input_defuzz_abs_event
syzkaller triggered a UBCAN warning:
[ 196.188950] UBSAN: Undefined behaviour in drivers/input/input.c:62:23
[ 196.188958] signed integer overflow:
[ 196.188964] -2147483647 - 104 cannot be represented in type 'int [2]'
[ 196.188973] CPU: 7 PID: 4763 Comm: syz-executor Not tainted
4.19.0-514.55.6.9.x86_64+ #7
[ 196.188977] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[ 196.188979] Call Trace:
[ 196.189001] dump_stack+0x91/0xeb
[ 196.189014] ubsan_epilogue+0x9/0x7c
[ 196.189020] handle_overflow+0x1d7/0x22c
[ 196.189028] ? __ubsan_handle_negate_overflow+0x18f/0x18f
[ 196.189038] ? __mutex_lock+0x213/0x13f0
[ 196.189053] ? drop_futex_key_refs+0xa0/0xa0
[ 196.189070] ? __might_fault+0xef/0x1b0
[ 196.189096] input_handle_event+0xe1b/0x1290
[ 196.189108] input_inject_event+0x1d7/0x27e
[ 196.189119] evdev_write+0x2cf/0x3f0
[ 196.189129] ? evdev_pass_values+0xd40/0xd40
[ 196.189157] ? mark_held_locks+0x160/0x160
[ 196.189171] ? __vfs_write+0xe0/0x6c0
[ 196.189175] ? evdev_pass_values+0xd40/0xd40
[ 196.189179] __vfs_write+0xe0/0x6c0
[ 196.189186] ? kernel_read+0x130/0x130
[ 196.189204] ? _cond_resched+0x15/0x30
[ 196.189214] ? __inode_security_revalidate+0xb8/0xe0
[ 196.189222] ? selinux_file_permission+0x354/0x430
[ 196.189233] vfs_write+0x160/0x440
[ 196.189242] ksys_write+0xc1/0x190
[ 196.189248] ? __ia32_sys_read+0xb0/0xb0
[ 196.189259] ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 196.189267] ? do_syscall_64+0x22/0x4a0
[ 196.189276] do_syscall_64+0xa5/0x4a0
[ 196.189287] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 196.189293] RIP: 0033:0x44e7c9
[ 196.189299] Code: fc ff 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f 00
the syzkaller reproduce script(but can't reproduce it every time):
r0 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
0x1)
write$binfmt_elf64(r0, &(0x7f000240)={{0x7f, 0x45, 0x4c, 0x46, 0x40,
0x2, 0x2, 0x, 0x374c, 0x3, 0x0, 0x8001, 0x103,
0x40, 0x22e, 0x26, 0x1, 0x38, 0x2, 0xa23, 0x1, 0x2}, [{0x6474e557, 0x5,
0x6, 0x2, 0x9, 0x9, 0x6c3, 0x1ff}], "", [[], [], [], []]}, 0x478)
ioctl$EVIOCGSW(0x, 0x8040451b, &(0x7f40)=""/7)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
r1 = syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
0x1)
openat$smack_task_current(0xff9c,
&(0x7f40)='/proc/self/attr/current\x00', 0x2, 0x0)
ioctl$EVIOCSABS0(r1, 0x401845c0, &(0x7f00)={0x4, 0x1, 0x4,
0xd1, 0x81, 0x3})
eventfd(0x1ff)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2,
0x200)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
syz_open_dev$evdev(&(0x7f000100)='/dev/input/event#\x00', 0x2, 0x1)
Typecast int to long to fix the issue.
Signed-off-by: liujian
---
drivers/input/input.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaa..24615ef 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -59,14 +59,17 @@ static inline int is_event_supported(unsigned int code,
static int input_defuzz_abs_event(int value, int old_val, int fuzz)
{
if (fuzz) {
- if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2)
+ if (value > (long)old_val - fuzz / 2 &&
+ value < (long)old_val + fuzz / 2)
return old_val;
- if (value > old_val - fuzz && value < old_val + fuzz)
- return (old_val * 3 + value) / 4;
+ if (value > (long)old_val - fuzz &&
+ value < (long)old_val + fuzz)
+ return ((long)old_val * 3 + value) / 4;
- if (value > old_val - fuzz * 2 && value < old_val + fuzz * 2)
- return (old_val + value) / 2;
+ if (value > (long)old_val - fuzz * 2 &&
+ value < (long)old_val + fuzz * 2)
+ return ((long)old_val + value) / 2;
}
return value;
--
2.7.4
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi Wang cong,
After apply the patch, I did not hit the issue again.
Thank you~
Best Regards,
liujian
> -Original Message-
> From: Dingtianhong
> Sent: Monday, July 24, 2017 9:29 AM
> To: Cong Wang; liujian (CE)
> Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com;
> da...@davemloft.net; eduma...@google.com; will...@google.com;
> dan...@iogearbox.net; net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
>
>
> On 2017/7/24 9:09, Ding Tianhong wrote:
> >
> >
> > On 2017/7/24 1:03, Cong Wang wrote:
> >> On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) <liujia...@huawei.com> wrote:
> >>> Hi
> >>>
> >>> I find it caused by below steps:
> >>> 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 2. set
> >>> tp_block_nr to 0 Then pg_vec was freed, and we did not delete the
> >>> timer?
> >>
> >> Thanks for testing!
> >>
> >> Ah, I overlook the initialization case in my previous patch.
> >>
> >> How about the following one? Does it cover all the cases?
> >>
> >>
> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> >> 008bb34ee324..0615c2a950fa 100644
> >> --- a/net/packet/af_packet.c
> >> +++ b/net/packet/af_packet.c
> >> @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk,
> >> union tpacket_req_u *req_u,
> >> register_prot_hook(sk);
> >> }
> >> spin_unlock(>bind_lock);
> >> - if (closing && (po->tp_version > TPACKET_V2)) {
> >> + if (pg_vec && (po->tp_version > TPACKET_V2)) {
> >> /* Because we don't support block-based V3 on tx-ring
> */
> >> if (!tx_ring)
> >> prb_shutdown_retire_blk_timer(po,
> rb_queue);
> >>
> >> .
> >
> > Hi, Cong:
> >
> > It looks like could not cover the case: req->tp_block_nr = 2 ->
> reg->tp_block_nr = 1 .
> >
>
> Oh, looks like this case would never happen, so I think your solution is ok.
>
> > what about this way:
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -4331,13 +4331,17 @@ static int packet_set_ring(struct sock *sk, union
> tpacket_req_u *req_u,
> > register_prot_hook(sk);
> > }
> > spin_unlock(>bind_lock);
> > - if (closing && (po->tp_version > TPACKET_V2)) {
> > + if ((closing || (pg_vec && !reg->tp_block_nr))&&
> > + (po->tp_version > TPACKET_V2)) {
> > /* Because we don't support block-based V3 on tx-ring */
> > if (!tx_ring)
> > prb_shutdown_retire_blk_timer(po,
> rb_queue);
> >
> >
>
> >>
> >
> >
> > .
> >
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi Wang cong,
After apply the patch, I did not hit the issue again.
Thank you~
Best Regards,
liujian
> -Original Message-
> From: Dingtianhong
> Sent: Monday, July 24, 2017 9:29 AM
> To: Cong Wang; liujian (CE)
> Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com;
> da...@davemloft.net; eduma...@google.com; will...@google.com;
> dan...@iogearbox.net; net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
>
>
> On 2017/7/24 9:09, Ding Tianhong wrote:
> >
> >
> > On 2017/7/24 1:03, Cong Wang wrote:
> >> On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) wrote:
> >>> Hi
> >>>
> >>> I find it caused by below steps:
> >>> 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 2. set
> >>> tp_block_nr to 0 Then pg_vec was freed, and we did not delete the
> >>> timer?
> >>
> >> Thanks for testing!
> >>
> >> Ah, I overlook the initialization case in my previous patch.
> >>
> >> How about the following one? Does it cover all the cases?
> >>
> >>
> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> >> 008bb34ee324..0615c2a950fa 100644
> >> --- a/net/packet/af_packet.c
> >> +++ b/net/packet/af_packet.c
> >> @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk,
> >> union tpacket_req_u *req_u,
> >> register_prot_hook(sk);
> >> }
> >> spin_unlock(>bind_lock);
> >> - if (closing && (po->tp_version > TPACKET_V2)) {
> >> + if (pg_vec && (po->tp_version > TPACKET_V2)) {
> >> /* Because we don't support block-based V3 on tx-ring
> */
> >> if (!tx_ring)
> >> prb_shutdown_retire_blk_timer(po,
> rb_queue);
> >>
> >> .
> >
> > Hi, Cong:
> >
> > It looks like could not cover the case: req->tp_block_nr = 2 ->
> reg->tp_block_nr = 1 .
> >
>
> Oh, looks like this case would never happen, so I think your solution is ok.
>
> > what about this way:
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -4331,13 +4331,17 @@ static int packet_set_ring(struct sock *sk, union
> tpacket_req_u *req_u,
> > register_prot_hook(sk);
> > }
> > spin_unlock(>bind_lock);
> > - if (closing && (po->tp_version > TPACKET_V2)) {
> > + if ((closing || (pg_vec && !reg->tp_block_nr))&&
> > + (po->tp_version > TPACKET_V2)) {
> > /* Because we don't support block-based V3 on tx-ring */
> > if (!tx_ring)
> > prb_shutdown_retire_blk_timer(po,
> rb_queue);
> >
> >
>
> >>
> >
> >
> > .
> >
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi I find it caused by below steps: 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 2. set tp_block_nr to 0 Then pg_vec was freed, and we did not delete the timer? Best Regards, liujian > -Original Message- > From: liujian (CE) > Sent: Sunday, July 23, 2017 5:47 PM > To: liujian (CE); 'Cong Wang'; Dingtianhong > Cc: 'Willem de Bruijn'; 'Dave Jones'; 'alexander.le...@verizon.com'; > 'da...@davemloft.net'; 'eduma...@google.com'; 'will...@google.com'; > 'dan...@iogearbox.net'; 'net...@vger.kernel.org'; > 'linux-kernel@vger.kernel.org' > Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired > > Hi, > > Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to > TPACKET_V1 ? > > > Best Regards, > liujian > > > > -Original Message- > > From: liujian (CE) > > Sent: Sunday, July 23, 2017 4:21 PM > > To: 'Cong Wang'; Dingtianhong > > Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com; > > da...@davemloft.net; eduma...@google.com; will...@google.com; > > dan...@iogearbox.net; net...@vger.kernel.org; > > linux-kernel@vger.kernel.org > > Subject: RE: af_packet: use after free in > > prb_retire_rx_blk_timer_expired > > > > Hi Wang Cong, > > > > With this patch , the system was crashed when setsockopt. > > > > The call trace as below: > > > > crash> bt > > PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main" > > #0 [8801bec03ce0] machine_kexec at 8105354b > > #1 [8801bec03d40] crash_kexec at 810f7e82 > > #2 [8801bec03e10] panic at 81650058 > > #3 [8801bec03e90] watchdog_timer_fn at 81122533 > > #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2 > > #5 [8801bec03f20] hrtimer_interrupt at 810ac450 > > #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457 > > #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0 > > #8 [8801bec03fb0] apic_timer_interrupt at 8166931d > > --- --- > > #9 [8801b301fcb8] apic_timer_interrupt at 8166931d > > [exception RIP: lock_timer_base+77] > > RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246 > > RAX: RBX: 8800afcc RCX: > > 0001 > > RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8 > > RBP: 8801b301fd80 R8: 8800afcc R9: > ea000268 > > R10: 003c R11: 8801b301fb2e R12: > 8800afcc > > R13: 8800afcc R14: R15: > 83d1a340 > > ORIG_RAX: ff10 CS: 0010 SS: 0018 > > #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f > > #11 [8801b301fdb8] del_timer_sync at 8108f252 > > #12 [8801b301fdd0] packet_set_ring at 81635e60 > > #13 [8801b301fe98] packet_setsockopt at 81636760 > > #14 [8801b301ff38] sys_setsockopt at 81531860 > > #15 [8801b301ff80] tracesys at 816687ed (via system_call) > > RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202 > > RAX: ffda RBX: 816687ed RCX: > > RDX: 0005 RSI: 00000107 RDI: > > 0180 > > RBP: 0180 R8: 001c R9: > > 7fcc78dc7160 > > R10: 01fd6ba0 R11: 0202 R12: > > > > R13: 0011 R14: 01fd6b60 R15: > > 01fd6b70 > > ORIG_RAX: 0036 CS: 0033 SS: 002b > > > > > > Best Regards, > > liujian > > > > > > > -Original Message- > > > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > > > Sent: Sunday, July 23, 2017 1:59 PM > > > To: Dingtianhong > > > Cc: liujian (CE); Willem de Bruijn; Dave Jones; > > > alexander.le...@verizon.com; da...@davemloft.net; > > eduma...@google.com; > > > will...@google.com; dan...@iogearbox.net; net...@vger.kernel.org; > > > linux-kernel@vger.kernel.org > > > Subject: Re: af_packet: use after free in > > > prb_retire_rx_blk_timer_expired > > > > > > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong > > > <dingtianh...@huawei.com> > > > wrote: > > > > Hi, Cong: > > > > > > > > Thanks for your quirk solution, but I still has some doubts about > > > > it, it looks like fix the problem in the &
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi I find it caused by below steps: 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 2. set tp_block_nr to 0 Then pg_vec was freed, and we did not delete the timer? Best Regards, liujian > -Original Message- > From: liujian (CE) > Sent: Sunday, July 23, 2017 5:47 PM > To: liujian (CE); 'Cong Wang'; Dingtianhong > Cc: 'Willem de Bruijn'; 'Dave Jones'; 'alexander.le...@verizon.com'; > 'da...@davemloft.net'; 'eduma...@google.com'; 'will...@google.com'; > 'dan...@iogearbox.net'; 'net...@vger.kernel.org'; > 'linux-kernel@vger.kernel.org' > Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired > > Hi, > > Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to > TPACKET_V1 ? > > > Best Regards, > liujian > > > > -Original Message- > > From: liujian (CE) > > Sent: Sunday, July 23, 2017 4:21 PM > > To: 'Cong Wang'; Dingtianhong > > Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com; > > da...@davemloft.net; eduma...@google.com; will...@google.com; > > dan...@iogearbox.net; net...@vger.kernel.org; > > linux-kernel@vger.kernel.org > > Subject: RE: af_packet: use after free in > > prb_retire_rx_blk_timer_expired > > > > Hi Wang Cong, > > > > With this patch , the system was crashed when setsockopt. > > > > The call trace as below: > > > > crash> bt > > PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main" > > #0 [8801bec03ce0] machine_kexec at 8105354b > > #1 [8801bec03d40] crash_kexec at 810f7e82 > > #2 [8801bec03e10] panic at 81650058 > > #3 [8801bec03e90] watchdog_timer_fn at 81122533 > > #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2 > > #5 [8801bec03f20] hrtimer_interrupt at 810ac450 > > #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457 > > #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0 > > #8 [8801bec03fb0] apic_timer_interrupt at 8166931d > > --- --- > > #9 [8801b301fcb8] apic_timer_interrupt at 8166931d > > [exception RIP: lock_timer_base+77] > > RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246 > > RAX: RBX: 8800afcc RCX: > > 0001 > > RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8 > > RBP: 8801b301fd80 R8: 8800afcc R9: > ea000268 > > R10: 003c R11: 8801b301fb2e R12: > 8800afcc > > R13: 8800afcc R14: R15: > 83d1a340 > > ORIG_RAX: ff10 CS: 0010 SS: 0018 > > #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f > > #11 [8801b301fdb8] del_timer_sync at 8108f252 > > #12 [8801b301fdd0] packet_set_ring at 81635e60 > > #13 [8801b301fe98] packet_setsockopt at 81636760 > > #14 [8801b301ff38] sys_setsockopt at 81531860 > > #15 [8801b301ff80] tracesys at 816687ed (via system_call) > > RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202 > > RAX: ffda RBX: 816687ed RCX: > > RDX: 0005 RSI: 00000107 RDI: > > 0180 > > RBP: 0180 R8: 001c R9: > > 7fcc78dc7160 > > R10: 01fd6ba0 R11: 0202 R12: > > > > R13: 0011 R14: 01fd6b60 R15: > > 01fd6b70 > > ORIG_RAX: 0036 CS: 0033 SS: 002b > > > > > > Best Regards, > > liujian > > > > > > > -Original Message- > > > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > > > Sent: Sunday, July 23, 2017 1:59 PM > > > To: Dingtianhong > > > Cc: liujian (CE); Willem de Bruijn; Dave Jones; > > > alexander.le...@verizon.com; da...@davemloft.net; > > eduma...@google.com; > > > will...@google.com; dan...@iogearbox.net; net...@vger.kernel.org; > > > linux-kernel@vger.kernel.org > > > Subject: Re: af_packet: use after free in > > > prb_retire_rx_blk_timer_expired > > > > > > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong > > > > > > wrote: > > > > Hi, Cong: > > > > > > > > Thanks for your quirk solution, but I still has some doubts about > > > > it, it looks like fix the problem in the > > > > packet_setsockopt->p
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi,
Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to
TPACKET_V1 ?
Best Regards,
liujian
> -Original Message-
> From: liujian (CE)
> Sent: Sunday, July 23, 2017 4:21 PM
> To: 'Cong Wang'; Dingtianhong
> Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com;
> da...@davemloft.net; eduma...@google.com; will...@google.com;
> dan...@iogearbox.net; net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
> Hi Wang Cong,
>
> With this patch , the system was crashed when setsockopt.
>
> The call trace as below:
>
> crash> bt
> PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main"
> #0 [8801bec03ce0] machine_kexec at 8105354b
> #1 [8801bec03d40] crash_kexec at 810f7e82
> #2 [8801bec03e10] panic at 81650058
> #3 [8801bec03e90] watchdog_timer_fn at 81122533
> #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2
> #5 [8801bec03f20] hrtimer_interrupt at 810ac450
> #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457
> #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0
> #8 [8801bec03fb0] apic_timer_interrupt at 8166931d
> --- ---
> #9 [8801b301fcb8] apic_timer_interrupt at 8166931d
> [exception RIP: lock_timer_base+77]
> RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246
> RAX: RBX: 8800afcc RCX:
> 0001
> RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8
> RBP: 8801b301fd80 R8: 8800afcc R9: ea000268
> R10: 003c R11: 8801b301fb2e R12: 8800afcc
> R13: 8800afcc R14: R15: 83d1a340
> ORIG_RAX: ff10 CS: 0010 SS: 0018
> #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f
> #11 [8801b301fdb8] del_timer_sync at 8108f252
> #12 [8801b301fdd0] packet_set_ring at 81635e60
> #13 [8801b301fe98] packet_setsockopt at 81636760
> #14 [8801b301ff38] sys_setsockopt at 81531860
> #15 [8801b301ff80] tracesys at 816687ed (via system_call)
> RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202
> RAX: ffda RBX: 816687ed RCX:
> RDX: 0005 RSI: 0107 RDI:
> 0180
> RBP: 0180 R8: 001c R9:
> 7fcc78dc7160
> R10: 01fd6ba0 R11: 0202 R12:
> 0000
> R13: 0011 R14: 01fd6b60 R15:
> 01fd6b70
> ORIG_RAX: 0036 CS: 0033 SS: 002b
>
>
> Best Regards,
> liujian
>
>
> > -Original Message-
> > From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> > Sent: Sunday, July 23, 2017 1:59 PM
> > To: Dingtianhong
> > Cc: liujian (CE); Willem de Bruijn; Dave Jones;
> > alexander.le...@verizon.com; da...@davemloft.net;
> eduma...@google.com;
> > will...@google.com; dan...@iogearbox.net; net...@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: af_packet: use after free in
> > prb_retire_rx_blk_timer_expired
> >
> > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong
> > <dingtianh...@huawei.com>
> > wrote:
> > > Hi, Cong:
> > >
> > > Thanks for your quirk solution, but I still has some doubts about
> > > it, it looks like fix the problem in the
> > > packet_setsockopt->packet_set_ring processing, but when in
> > > packet_release processing, it may could not release the real pg_vec
> > > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss
> > > something here, nice to hear from your feedback. :)
> >
> > Yes you miss that packet_release() has memset()'s so we won't hit that
> > path. :)
> >
> > However, I missed the swap() in this messy function, actually I
> > believe the bug is that we modify tpacket_kbdq_core inside rx_ring in
> > non-closing case without actually stopping its timer. I feel more confident
> with the following patch:
> >
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> > 008bb34ee324..267b181fef15 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk,
> > union tpacket_req_u *req_u,
> > case TPACKET_V3:
> > /* Block transmit is not supported yet */
> > if (!tx_ring) {
> > + prb_shutdown_retire_blk_timer(po,
> > + rb_queue);
> > init_prb_bdqc(po, rb, pg_vec, req_u);
> > } else {
> > struct tpacket_req3 *req3 =
> > _u->req3;
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi,
Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to
TPACKET_V1 ?
Best Regards,
liujian
> -Original Message-
> From: liujian (CE)
> Sent: Sunday, July 23, 2017 4:21 PM
> To: 'Cong Wang'; Dingtianhong
> Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com;
> da...@davemloft.net; eduma...@google.com; will...@google.com;
> dan...@iogearbox.net; net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
> Hi Wang Cong,
>
> With this patch , the system was crashed when setsockopt.
>
> The call trace as below:
>
> crash> bt
> PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main"
> #0 [8801bec03ce0] machine_kexec at 8105354b
> #1 [8801bec03d40] crash_kexec at 810f7e82
> #2 [8801bec03e10] panic at 81650058
> #3 [8801bec03e90] watchdog_timer_fn at 81122533
> #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2
> #5 [8801bec03f20] hrtimer_interrupt at 810ac450
> #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457
> #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0
> #8 [8801bec03fb0] apic_timer_interrupt at 8166931d
> --- ---
> #9 [8801b301fcb8] apic_timer_interrupt at 8166931d
> [exception RIP: lock_timer_base+77]
> RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246
> RAX: RBX: 8800afcc RCX:
> 0001
> RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8
> RBP: 8801b301fd80 R8: 8800afcc R9: ea000268
> R10: 003c R11: 8801b301fb2e R12: 8800afcc
> R13: 8800afcc R14: R15: 83d1a340
> ORIG_RAX: ff10 CS: 0010 SS: 0018
> #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f
> #11 [8801b301fdb8] del_timer_sync at 8108f252
> #12 [8801b301fdd0] packet_set_ring at 81635e60
> #13 [8801b301fe98] packet_setsockopt at 81636760
> #14 [8801b301ff38] sys_setsockopt at 81531860
> #15 [8801b301ff80] tracesys at 816687ed (via system_call)
> RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202
> RAX: ffda RBX: 816687ed RCX:
> RDX: 0005 RSI: 0107 RDI:
> 0180
> RBP: 0180 R8: 001c R9:
> 7fcc78dc7160
> R10: 01fd6ba0 R11: 0202 R12:
> 0000
> R13: 0011 R14: 01fd6b60 R15:
> 01fd6b70
> ORIG_RAX: 0036 CS: 0033 SS: 002b
>
>
> Best Regards,
> liujian
>
>
> > -Original Message-
> > From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> > Sent: Sunday, July 23, 2017 1:59 PM
> > To: Dingtianhong
> > Cc: liujian (CE); Willem de Bruijn; Dave Jones;
> > alexander.le...@verizon.com; da...@davemloft.net;
> eduma...@google.com;
> > will...@google.com; dan...@iogearbox.net; net...@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: af_packet: use after free in
> > prb_retire_rx_blk_timer_expired
> >
> > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong
> >
> > wrote:
> > > Hi, Cong:
> > >
> > > Thanks for your quirk solution, but I still has some doubts about
> > > it, it looks like fix the problem in the
> > > packet_setsockopt->packet_set_ring processing, but when in
> > > packet_release processing, it may could not release the real pg_vec
> > > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss
> > > something here, nice to hear from your feedback. :)
> >
> > Yes you miss that packet_release() has memset()'s so we won't hit that
> > path. :)
> >
> > However, I missed the swap() in this messy function, actually I
> > believe the bug is that we modify tpacket_kbdq_core inside rx_ring in
> > non-closing case without actually stopping its timer. I feel more confident
> with the following patch:
> >
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> > 008bb34ee324..267b181fef15 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk,
> > union tpacket_req_u *req_u,
> > case TPACKET_V3:
> > /* Block transmit is not supported yet */
> > if (!tx_ring) {
> > + prb_shutdown_retire_blk_timer(po,
> > + rb_queue);
> > init_prb_bdqc(po, rb, pg_vec, req_u);
> > } else {
> > struct tpacket_req3 *req3 =
> > _u->req3;
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi Wang Cong,
With this patch , the system was crashed when setsockopt.
The call trace as below:
crash> bt
PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main"
#0 [8801bec03ce0] machine_kexec at 8105354b
#1 [8801bec03d40] crash_kexec at 810f7e82
#2 [8801bec03e10] panic at 81650058
#3 [8801bec03e90] watchdog_timer_fn at 81122533
#4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2
#5 [8801bec03f20] hrtimer_interrupt at 810ac450
#6 [8801bec03f70] local_apic_timer_interrupt at 8104a457
#7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0
#8 [8801bec03fb0] apic_timer_interrupt at 8166931d
--- ---
#9 [8801b301fcb8] apic_timer_interrupt at 8166931d
[exception RIP: lock_timer_base+77]
RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246
RAX: RBX: 8800afcc RCX: 0001
RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8
RBP: 8801b301fd80 R8: 8800afcc R9: ea000268
R10: 003c R11: 8801b301fb2e R12: 8800afcc
R13: 8800afcc R14: R15: 83d1a340
ORIG_RAX: ff10 CS: 0010 SS: 0018
#10 [8801b301fd88] try_to_del_timer_sync at 8108f19f
#11 [8801b301fdb8] del_timer_sync at 8108f252
#12 [8801b301fdd0] packet_set_ring at 81635e60
#13 [8801b301fe98] packet_setsockopt at 81636760
#14 [8801b301ff38] sys_setsockopt at 81531860
#15 [8801b301ff80] tracesys at 816687ed (via system_call)
RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202
RAX: ffda RBX: 816687ed RCX:
RDX: 0005 RSI: 0107 RDI: 0180
RBP: 0180 R8: 001c R9: 7fcc78dc7160
R10: 01fd6ba0 R11: 0202 R12:
R13: 0011 R14: 01fd6b60 R15: 01fd6b70
ORIG_RAX: 0036 CS: 0033 SS: 002b
Best Regards,
liujian
> -Original Message-
> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> Sent: Sunday, July 23, 2017 1:59 PM
> To: Dingtianhong
> Cc: liujian (CE); Willem de Bruijn; Dave Jones; alexander.le...@verizon.com;
> da...@davemloft.net; eduma...@google.com; will...@google.com;
> dan...@iogearbox.net; net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
> On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong <dingtianh...@huawei.com>
> wrote:
> > Hi, Cong:
> >
> > Thanks for your quirk solution, but I still has some doubts about it,
> > it looks like fix the problem in the
> > packet_setsockopt->packet_set_ring processing, but when in
> > packet_release processing, it may could not release the real pg_vec
> > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss
> > something here, nice to hear from your feedback. :)
>
> Yes you miss that packet_release() has memset()'s so we won't hit that path.
> :)
>
> However, I missed the swap() in this messy function, actually I believe the
> bug
> is that we modify tpacket_kbdq_core inside rx_ring in non-closing case without
> actually stopping its timer. I feel more confident with the following patch:
>
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> 008bb34ee324..267b181fef15 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk, union
> tpacket_req_u *req_u,
> case TPACKET_V3:
> /* Block transmit is not supported yet */
> if (!tx_ring) {
> + prb_shutdown_retire_blk_timer(po,
> + rb_queue);
> init_prb_bdqc(po, rb, pg_vec, req_u);
> } else {
> struct tpacket_req3 *req3 =
> _u->req3;
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi Wang Cong,
With this patch , the system was crashed when setsockopt.
The call trace as below:
crash> bt
PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main"
#0 [8801bec03ce0] machine_kexec at 8105354b
#1 [8801bec03d40] crash_kexec at 810f7e82
#2 [8801bec03e10] panic at 81650058
#3 [8801bec03e90] watchdog_timer_fn at 81122533
#4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2
#5 [8801bec03f20] hrtimer_interrupt at 810ac450
#6 [8801bec03f70] local_apic_timer_interrupt at 8104a457
#7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0
#8 [8801bec03fb0] apic_timer_interrupt at 8166931d
--- ---
#9 [8801b301fcb8] apic_timer_interrupt at 8166931d
[exception RIP: lock_timer_base+77]
RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246
RAX: RBX: 8800afcc RCX: 0001
RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8
RBP: 8801b301fd80 R8: 8800afcc R9: ea000268
R10: 003c R11: 8801b301fb2e R12: 8800afcc
R13: 8800afcc R14: R15: 83d1a340
ORIG_RAX: ff10 CS: 0010 SS: 0018
#10 [8801b301fd88] try_to_del_timer_sync at 8108f19f
#11 [8801b301fdb8] del_timer_sync at 8108f252
#12 [8801b301fdd0] packet_set_ring at 81635e60
#13 [8801b301fe98] packet_setsockopt at 81636760
#14 [8801b301ff38] sys_setsockopt at 81531860
#15 [8801b301ff80] tracesys at 816687ed (via system_call)
RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202
RAX: ffda RBX: 816687ed RCX:
RDX: 0005 RSI: 0107 RDI: 0180
RBP: 0180 R8: 001c R9: 7fcc78dc7160
R10: 01fd6ba0 R11: 0202 R12:
R13: 0011 R14: 01fd6b60 R15: 01fd6b70
ORIG_RAX: 0036 CS: 0033 SS: 002b
Best Regards,
liujian
> -Original Message-
> From: Cong Wang [mailto:xiyou.wangc...@gmail.com]
> Sent: Sunday, July 23, 2017 1:59 PM
> To: Dingtianhong
> Cc: liujian (CE); Willem de Bruijn; Dave Jones; alexander.le...@verizon.com;
> da...@davemloft.net; eduma...@google.com; will...@google.com;
> dan...@iogearbox.net; net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
> On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong
> wrote:
> > Hi, Cong:
> >
> > Thanks for your quirk solution, but I still has some doubts about it,
> > it looks like fix the problem in the
> > packet_setsockopt->packet_set_ring processing, but when in
> > packet_release processing, it may could not release the real pg_vec
> > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss
> > something here, nice to hear from your feedback. :)
>
> Yes you miss that packet_release() has memset()'s so we won't hit that path.
> :)
>
> However, I missed the swap() in this messy function, actually I believe the
> bug
> is that we modify tpacket_kbdq_core inside rx_ring in non-closing case without
> actually stopping its timer. I feel more confident with the following patch:
>
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index
> 008bb34ee324..267b181fef15 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk, union
> tpacket_req_u *req_u,
> case TPACKET_V3:
> /* Block transmit is not supported yet */
> if (!tx_ring) {
> + prb_shutdown_retire_blk_timer(po,
> + rb_queue);
> init_prb_bdqc(po, rb, pg_vec, req_u);
> } else {
> struct tpacket_req3 *req3 =
> _u->req3;
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
I also hit this issue with trinity test:
The call trace:
[exception RIP: prb_retire_rx_blk_timer_expired+70]
RIP: 81633be6 RSP: 8801bec03dc0 RFLAGS: 00010246
RAX: RBX: 8801b49d0948 RCX:
RDX: 8801b31057a0 RSI: a56b6b6b6b6b6b6b RDI: 8801b49d09ec
RBP: 8801bec03dd8 R8: 0001 R9: 83e1bf80
R10: 0002 R11: 0005 R12: 8801b49d09ec
R13: 0100 R14: 81633ba0 R15: 8801b49d0948
ORIG_RAX: CS: 0010 SS: 0018
#7 [8801bec03de0] call_timer_fn at 8108cb76
#8 [8801bec03e18] run_timer_softirq at 8108f87c
#9 [8801bec03e90] __do_softirq at 8108629f
#10 [8801bec03f00] call_softirq at 8166a01c
#11 [8801bec03f18] do_softirq at 810172ad
#12 [8801bec03f30] irq_exit at 81086655
#13 [8801bec03f48] msa_irq_exit at 810b1ab3
#14 [8801bec03f88] smp_apic_timer_interrupt at 8166aeae
#15 [8801bec03fb0] apic_timer_interrupt at 816692dd
--- ---
And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); is
a56b6b6b6b6b6b6b
struct packet_ring_buffer rx_ring = {
pg_vec = 0x0,
head = 0x0,
frames_per_block = 0x400,
frame_size = 0x0,
frame_max = 0x,
pg_vec_order = 0x0,
pg_vec_pages = 0x0,
pg_vec_len = 0x0,
pending_refcnt = 0x0,
prb_bdqc = {
pkbdq = 0x8801b31057a0,
feature_req_word = 0x1,
hdrlen = 0x44,
reset_pending_on_curr_blk = 0x1,
delete_blk_timer = 0x0,
kactive_blk_num = 0x0,
blk_sizeof_priv = 0x0,
last_kactive_blk_num = 0x0,
pkblk_start = 0x8800a700 struct: page excluded: kernel virtual
address: 8800a700 type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: 8800a700 type:
"gdb_readmem_callback"
,
pkblk_end = 0x8800a720 "\002",
kblk_size = 0x20,
max_frame_len = 0x1fffd0,
knum_blocks = 0x1,
knxt_seq_num = 0x2,
prev = 0x8800a730 struct: page excluded: kernel virtual address:
8800a730 type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: 8800a730 type:
"gdb_readmem_callback"
,
nxt_offset = 0x8800a730 struct: page excluded: kernel virtual
address: 8800a730 type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: 8800a730 type:
"gdb_readmem_callback"
,
skb = 0x0,
blk_fill_in_prog = {
counter = 0x0
crash> struct pgv 0x8801b31057a0
struct pgv {
buffer = 0xa56b6b6b6b6b6b6b
}
Best Regards,
liujian
> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Willem de Bruijn
> Sent: Wednesday, April 12, 2017 7:23 AM
> To: Dave Jones; alexander.le...@verizon.com; da...@davemloft.net;
> eduma...@google.com; will...@google.com; dan...@iogearbox.net;
> net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
> On Mon, Apr 10, 2017 at 3:23 PM, Dave Jones <da...@codemonkey.org.uk>
> wrote:
> > On Mon, Apr 10, 2017 at 07:03:30PM +, alexander.le...@verizon.com
> wrote:
> > > Hi all,
> > >
> > > I seem to be hitting this use-after-free on a -next kernel using trinity:
> > >
> > > [ 531.036054] BUG: KASAN: use-after-free in
> > prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688)
>
> The retire_blk_timer is called after the pg_vec struct for this ring was
> freed.
> This should not happen. packet_set_ring stops the timer with del_timer_sync
> when tearing down the ring before freeing that
> struct:
>
> if (closing && (po->tp_version > TPACKET_V2)) {
> /* Because we don't support block-based V3 on tx-ring */
> if (!tx_ring)
> prb_shutdown_retire_blk_timer(po, rb_queue);
> }
>
> if (pg_vec)
> free_pg_vec(pg_vec, order, req->tp_block_nr);
>
> This is a similar race to the use-after-free fixed by 84ac7260236a
> ("packet: fix race condition in packet_set_ring"). The previous race was
> triggered by a call to setsockopt PACKET_VERSION changing tp_version while
> the ring is active. It is not immediately obvious what is the cause now. I
> suppose trinity does not give a trace of such system calls on this file
> descriptor?
> That would be helpful.
>
> The bug report shows both a timer firing after the packet_set_ring call that
> freed the pg_vec, and later a CONFIG_DEBUG_OBJECTS_FREE warning that
> the timer is still active when the socket is closed on release of the last
> file
> descriptor.
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
I also hit this issue with trinity test:
The call trace:
[exception RIP: prb_retire_rx_blk_timer_expired+70]
RIP: 81633be6 RSP: 8801bec03dc0 RFLAGS: 00010246
RAX: RBX: 8801b49d0948 RCX:
RDX: 8801b31057a0 RSI: a56b6b6b6b6b6b6b RDI: 8801b49d09ec
RBP: 8801bec03dd8 R8: 0001 R9: 83e1bf80
R10: 0002 R11: 0005 R12: 8801b49d09ec
R13: 0100 R14: 81633ba0 R15: 8801b49d0948
ORIG_RAX: CS: 0010 SS: 0018
#7 [8801bec03de0] call_timer_fn at 8108cb76
#8 [8801bec03e18] run_timer_softirq at 8108f87c
#9 [8801bec03e90] __do_softirq at 8108629f
#10 [8801bec03f00] call_softirq at 8166a01c
#11 [8801bec03f18] do_softirq at 810172ad
#12 [8801bec03f30] irq_exit at 81086655
#13 [8801bec03f48] msa_irq_exit at 810b1ab3
#14 [8801bec03f88] smp_apic_timer_interrupt at 8166aeae
#15 [8801bec03fb0] apic_timer_interrupt at 816692dd
--- ---
And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); is
a56b6b6b6b6b6b6b
struct packet_ring_buffer rx_ring = {
pg_vec = 0x0,
head = 0x0,
frames_per_block = 0x400,
frame_size = 0x0,
frame_max = 0x,
pg_vec_order = 0x0,
pg_vec_pages = 0x0,
pg_vec_len = 0x0,
pending_refcnt = 0x0,
prb_bdqc = {
pkbdq = 0x8801b31057a0,
feature_req_word = 0x1,
hdrlen = 0x44,
reset_pending_on_curr_blk = 0x1,
delete_blk_timer = 0x0,
kactive_blk_num = 0x0,
blk_sizeof_priv = 0x0,
last_kactive_blk_num = 0x0,
pkblk_start = 0x8800a700 struct: page excluded: kernel virtual
address: 8800a700 type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: 8800a700 type:
"gdb_readmem_callback"
,
pkblk_end = 0x8800a720 "\002",
kblk_size = 0x20,
max_frame_len = 0x1fffd0,
knum_blocks = 0x1,
knxt_seq_num = 0x2,
prev = 0x8800a730 struct: page excluded: kernel virtual address:
8800a730 type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: 8800a730 type:
"gdb_readmem_callback"
,
nxt_offset = 0x8800a730 struct: page excluded: kernel virtual
address: 8800a730 type: "gdb_readmem_callback"
struct: page excluded: kernel virtual address: 8800a730 type:
"gdb_readmem_callback"
,
skb = 0x0,
blk_fill_in_prog = {
counter = 0x0
crash> struct pgv 0x8801b31057a0
struct pgv {
buffer = 0xa56b6b6b6b6b6b6b
}
Best Regards,
liujian
> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Willem de Bruijn
> Sent: Wednesday, April 12, 2017 7:23 AM
> To: Dave Jones; alexander.le...@verizon.com; da...@davemloft.net;
> eduma...@google.com; will...@google.com; dan...@iogearbox.net;
> net...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
>
> On Mon, Apr 10, 2017 at 3:23 PM, Dave Jones
> wrote:
> > On Mon, Apr 10, 2017 at 07:03:30PM +, alexander.le...@verizon.com
> wrote:
> > > Hi all,
> > >
> > > I seem to be hitting this use-after-free on a -next kernel using trinity:
> > >
> > > [ 531.036054] BUG: KASAN: use-after-free in
> > prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688)
>
> The retire_blk_timer is called after the pg_vec struct for this ring was
> freed.
> This should not happen. packet_set_ring stops the timer with del_timer_sync
> when tearing down the ring before freeing that
> struct:
>
> if (closing && (po->tp_version > TPACKET_V2)) {
> /* Because we don't support block-based V3 on tx-ring */
> if (!tx_ring)
> prb_shutdown_retire_blk_timer(po, rb_queue);
> }
>
> if (pg_vec)
> free_pg_vec(pg_vec, order, req->tp_block_nr);
>
> This is a similar race to the use-after-free fixed by 84ac7260236a
> ("packet: fix race condition in packet_set_ring"). The previous race was
> triggered by a call to setsockopt PACKET_VERSION changing tp_version while
> the ring is active. It is not immediately obvious what is the cause now. I
> suppose trinity does not give a trace of such system calls on this file
> descriptor?
> That would be helpful.
>
> The bug report shows both a timer firing after the packet_set_ring call that
> freed the pg_vec, and later a CONFIG_DEBUG_OBJECTS_FREE warning that
> the timer is still active when the socket is closed on release of the last
> file
> descriptor.
