diff mbox series

[4.19,016/125] media: pci: ttpci: av7110: fix possible buffer overflow caused by bad DMA value in debiirq()

Message ID 20200901150935.368387062@linuxfoundation.org
State Superseded
Headers show
Series None | expand

Commit Message

Greg KH Sept. 1, 2020, 3:09 p.m. UTC 
From: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>

[ Upstream commit 6499a0db9b0f1e903d52f8244eacc1d4be00eea2 ]

The value av7110->debi_virt is stored in DMA memory, and it is assigned
to data, and thus data[0] can be modified at any time by malicious
hardware. In this case, "if (data[0] < 2)" can be passed, but then
data[0] can be changed into a large number, which may cause buffer
overflow when the code "av7110->ci_slot[data[0]]" is used.

To fix this possible bug, data[0] is assigned to a local variable, which
replaces the use of data[0].

Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
Signed-off-by: Sean Young <sean@mess.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/media/pci/ttpci/av7110.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Sean Young Sept. 1, 2020, 4:25 p.m. UTC | #1 
Greg,

On Tue, Sep 01, 2020 at 05:09:31PM +0200, Greg Kroah-Hartman wrote:
> From: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
> 
> [ Upstream commit 6499a0db9b0f1e903d52f8244eacc1d4be00eea2 ]
> 
> The value av7110->debi_virt is stored in DMA memory, and it is assigned
> to data, and thus data[0] can be modified at any time by malicious
> hardware. In this case, "if (data[0] < 2)" can be passed, but then
> data[0] can be changed into a large number, which may cause buffer
> overflow when the code "av7110->ci_slot[data[0]]" is used.
> 
> To fix this possible bug, data[0] is assigned to a local variable, which
> replaces the use of data[0].

See the discussion here:

https://lkml.org/lkml/2020/8/31/479

It does not seem worthwhile merging to the stable trees.

Thanks

Sean

> 
> Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
> Signed-off-by: Sean Young <sean@mess.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/media/pci/ttpci/av7110.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/ttpci/av7110.c b/drivers/media/pci/ttpci/av7110.c
> index d6816effb8786..d02b5fd940c12 100644
> --- a/drivers/media/pci/ttpci/av7110.c
> +++ b/drivers/media/pci/ttpci/av7110.c
> @@ -424,14 +424,15 @@ static void debiirq(unsigned long cookie)
>  	case DATA_CI_GET:
>  	{
>  		u8 *data = av7110->debi_virt;
> +		u8 data_0 = data[0];
>  
> -		if ((data[0] < 2) && data[2] == 0xff) {
> +		if (data_0 < 2 && data[2] == 0xff) {
>  			int flags = 0;
>  			if (data[5] > 0)
>  				flags |= CA_CI_MODULE_PRESENT;
>  			if (data[5] > 5)
>  				flags |= CA_CI_MODULE_READY;
> -			av7110->ci_slot[data[0]].flags = flags;
> +			av7110->ci_slot[data_0].flags = flags;
>  		} else
>  			ci_get_data(&av7110->ci_rbuffer,
>  				    av7110->debi_virt,
> -- 
> 2.25.1
> 
>
Greg KH Sept. 1, 2020, 4:35 p.m. UTC | #2 
On Tue, Sep 01, 2020 at 05:25:12PM +0100, Sean Young wrote:
> Greg,
> 
> On Tue, Sep 01, 2020 at 05:09:31PM +0200, Greg Kroah-Hartman wrote:
> > From: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
> > 
> > [ Upstream commit 6499a0db9b0f1e903d52f8244eacc1d4be00eea2 ]
> > 
> > The value av7110->debi_virt is stored in DMA memory, and it is assigned
> > to data, and thus data[0] can be modified at any time by malicious
> > hardware. In this case, "if (data[0] < 2)" can be passed, but then
> > data[0] can be changed into a large number, which may cause buffer
> > overflow when the code "av7110->ci_slot[data[0]]" is used.
> > 
> > To fix this possible bug, data[0] is assigned to a local variable, which
> > replaces the use of data[0].
> 
> See the discussion here:
> 
> https://lkml.org/lkml/2020/8/31/479
> 
> It does not seem worthwhile merging to the stable trees.

It doesn't hurt either :)

thanks,

greg k-h
Pavel Machek Sept. 1, 2020, 9:16 p.m. UTC | #3 
On Tue 2020-09-01 18:35:23, Greg Kroah-Hartman wrote:
> On Tue, Sep 01, 2020 at 05:25:12PM +0100, Sean Young wrote:
> > Greg,
> > 
> > On Tue, Sep 01, 2020 at 05:09:31PM +0200, Greg Kroah-Hartman wrote:
> > > From: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
> > > 
> > > [ Upstream commit 6499a0db9b0f1e903d52f8244eacc1d4be00eea2 ]
> > > 
> > > The value av7110->debi_virt is stored in DMA memory, and it is assigned
> > > to data, and thus data[0] can be modified at any time by malicious
> > > hardware. In this case, "if (data[0] < 2)" can be passed, but then
> > > data[0] can be changed into a large number, which may cause buffer
> > > overflow when the code "av7110->ci_slot[data[0]]" is used.
> > > 
> > > To fix this possible bug, data[0] is assigned to a local variable, which
> > > replaces the use of data[0].
> > 
> > See the discussion here:
> > 
> > https://lkml.org/lkml/2020/8/31/479
> > 
> > It does not seem worthwhile merging to the stable trees.
> 
> It doesn't hurt either :)

Update stable kernel rules.

If "patch does not match description and is pretty obviously useless"
but "does not hurt" is acceptable for stable tree, people should know.

You are pushing known junk into stable. Stop that.
									Pavel
Jia-Ju Bai Sept. 2, 2020, 1:23 a.m. UTC | #4 
On 2020/9/2 5:16, Pavel Machek wrote:
> On Tue 2020-09-01 18:35:23, Greg Kroah-Hartman wrote:
>> On Tue, Sep 01, 2020 at 05:25:12PM +0100, Sean Young wrote:
>>> Greg,
>>>
>>> On Tue, Sep 01, 2020 at 05:09:31PM +0200, Greg Kroah-Hartman wrote:
>>>> From: Jia-Ju Bai <baijiaju@tsinghua.edu.cn>
>>>>
>>>> [ Upstream commit 6499a0db9b0f1e903d52f8244eacc1d4be00eea2 ]
>>>>
>>>> The value av7110->debi_virt is stored in DMA memory, and it is assigned
>>>> to data, and thus data[0] can be modified at any time by malicious
>>>> hardware. In this case, "if (data[0] < 2)" can be passed, but then
>>>> data[0] can be changed into a large number, which may cause buffer
>>>> overflow when the code "av7110->ci_slot[data[0]]" is used.
>>>>
>>>> To fix this possible bug, data[0] is assigned to a local variable, which
>>>> replaces the use of data[0].
>>> See the discussion here:
>>>
>>> https://lkml.org/lkml/2020/8/31/479
>>>
>>> It does not seem worthwhile merging to the stable trees.
>> It doesn't hurt either :)
> Update stable kernel rules.
>
> If "patch does not match description and is pretty obviously useless"
> but "does not hurt" is acceptable for stable tree, people should know.
>
> You are pushing known junk into stable. Stop that.

Sorry for my useless patch...

Recently I submitted a new patch wiith READ_ONCE() to fix the problem 
that Pavel said:
https://lkml.org/lkml/2020/8/30/67

If you think this new patch is still useless, reverting the code is fine 
to me.


Best wishes,
Jia-Ju Bai
diff mbox series

Patch

diff --git a/drivers/media/pci/ttpci/av7110.c b/drivers/media/pci/ttpci/av7110.c
index d6816effb8786..d02b5fd940c12 100644
--- a/drivers/media/pci/ttpci/av7110.c
+++ b/drivers/media/pci/ttpci/av7110.c
@@ -424,14 +424,15 @@  static void debiirq(unsigned long cookie)
 	case DATA_CI_GET:
 	{
 		u8 *data = av7110->debi_virt;
+		u8 data_0 = data[0];
 
-		if ((data[0] < 2) && data[2] == 0xff) {
+		if (data_0 < 2 && data[2] == 0xff) {
 			int flags = 0;
 			if (data[5] > 0)
 				flags |= CA_CI_MODULE_PRESENT;
 			if (data[5] > 5)
 				flags |= CA_CI_MODULE_READY;
-			av7110->ci_slot[data[0]].flags = flags;
+			av7110->ci_slot[data_0].flags = flags;
 		} else
 			ci_get_data(&av7110->ci_rbuffer,
 				    av7110->debi_virt,