| Message ID | 20200505142110.7620-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: usb: ttusb-dec: avoid buffer overflow in ttusb_dec_handle_irq() when DMA failures/attacks occur | expand |
On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote: > In ttusb_dec_init_usb(): > dec->irq_buffer = usb_alloc_coherent(...) Nice. > Thus, "dec->irq_buffer" is a DMA value, and it is assigned to "buffer" > in ttusb_dec_handle_irq(): > char *buffer = dec->irq_buffer; Nice. > When DMA failures or attacks occur, the value of buffer[4] can be > changed at any time. Wait, how can that happen? The kernel can not protect itself from something like that in each individual driver, that's impossible. Your patch just makes that "window" a few cycles smaller than before. > In this case, "buffer[4] - 1 < ARRAY_SIZE(rc_keys)" > can be first satisfied, and then the value of buffer[4] can be changed > to a large number, causing a buffer-overflow vulnerability. Um, maybe. I agree testing and then using the value can cause problems when userspace provides you with that data and control, but for DMA-backed memory, we are in so much other trouble if that is the case. > To avoid the risk of this vulnerability, buffer[4] is assigned to a > non-DMA local variable "index" at the beginning of > ttusb_dec_handle_irq(), and then this variable replaces each use of > buffer[4] in the function. I strongly doubt this is even possible. Can you describe how you can modify DMA memory and if so, would you do something tiny like this? > > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > drivers/media/usb/ttusb-dec/ttusb_dec.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/ttusb-dec/ttusb_dec.c b/drivers/media/usb/ttusb-dec/ttusb_dec.c > index 3198f9624b7c..8543c552515b 100644 > --- a/drivers/media/usb/ttusb-dec/ttusb_dec.c > +++ b/drivers/media/usb/ttusb-dec/ttusb_dec.c > @@ -250,6 +250,7 @@ static void ttusb_dec_handle_irq( struct urb *urb) > struct ttusb_dec *dec = urb->context; > char *buffer = dec->irq_buffer; > int retval; > + u8 index = buffer[4]; > > switch(urb->status) { > case 0: /*success*/ > @@ -281,11 +282,11 @@ static void ttusb_dec_handle_irq( struct urb *urb) > * this should/could be added later ... > * for now lets report each signal as a key down and up > */ > - if (buffer[4] - 1 < ARRAY_SIZE(rc_keys)) { > - dprintk("%s:rc signal:%d\n", __func__, buffer[4]); > - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 1); > + if (index - 1 < ARRAY_SIZE(rc_keys)) { > + dprintk("%s:rc signal:%d\n", __func__, index); > + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 1); > input_sync(dec->rc_input_dev); > - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 0); > + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 0); > input_sync(dec->rc_input_dev); > } > } The above complaints aside, the patch does make sense for the simple fact that it might reduce the number of memory accesses. But, the compiler might already be doing this type of optimization anyway, right? So your original issue might not even be a problem. Anyhow, the patch looks fine, but it's a minor cleanup, not a fix for any sort of bug/security issue at all. If you change the text in the changelog area, I'll be glad to ack it. thanks, greg k-h
Hi Greg, Thanks for the reply :) On 2020/5/6 2:10, Greg KH wrote: > On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote: >> In this case, "buffer[4] - 1 < ARRAY_SIZE(rc_keys)" >> can be first satisfied, and then the value of buffer[4] can be changed >> to a large number, causing a buffer-overflow vulnerability. > Um, maybe. I agree testing and then using the value can cause problems > when userspace provides you with that data and control, but for > DMA-backed memory, we are in so much other trouble if that is the case. > >> To avoid the risk of this vulnerability, buffer[4] is assigned to a >> non-DMA local variable "index" at the beginning of >> ttusb_dec_handle_irq(), and then this variable replaces each use of >> buffer[4] in the function. > I strongly doubt this is even possible. Can you describe how you can > modify DMA memory and if so, would you do something tiny like this? > I have never modified DMA memory in the real world, but an attacker can use a malicious device to do this. There is a video that shows how to use the Inception tool to perform DMA attacks and login in the Windows OS without password: https://www.youtube.com/watch?v=HDhpy7RpUjM Besides, the Windows OS resists against DMA attacks by disabling DMA of external devices by default: http://support.microsoft.com/kb/2516445 Considering that this patch is for a USB media driver, I think that an attacker can just insert a malicious USB device to modify DMA memory and trigger this bug. Besides, not related to this patch, some drivers use DMA to send/receive data (such as the URB used in USB drivers and ring descriptors used in network drivers). In this case, if the data is malicious and used as an array index through DMA, security problems may occur. In my opinion, similar to the data from userspace, the data from hardware may be also malicious and should be checked. Maybe we could discuss this issue with DMA driver developers? Best wishes, Jia-Ju Bai
On Wed, May 06, 2020 at 06:13:01PM +0800, Jia-Ju Bai wrote: > Hi Greg, > > Thanks for the reply :) > > On 2020/5/6 2:10, Greg KH wrote: > > On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote: > > > In this case, "buffer[4] - 1 < ARRAY_SIZE(rc_keys)" > > > can be first satisfied, and then the value of buffer[4] can be changed > > > to a large number, causing a buffer-overflow vulnerability. > > Um, maybe. I agree testing and then using the value can cause problems > > when userspace provides you with that data and control, but for > > DMA-backed memory, we are in so much other trouble if that is the case. > > > > > To avoid the risk of this vulnerability, buffer[4] is assigned to a > > > non-DMA local variable "index" at the beginning of > > > ttusb_dec_handle_irq(), and then this variable replaces each use of > > > buffer[4] in the function. > > I strongly doubt this is even possible. Can you describe how you can > > modify DMA memory and if so, would you do something tiny like this? > > > > I have never modified DMA memory in the real world, but an attacker can use > a malicious device to do this. > There is a video that shows how to use the Inception tool to perform DMA > attacks and login in the Windows OS without password: > https://www.youtube.com/watch?v=HDhpy7RpUjM If you have control over the hardware, and can write to any DMA memory, again, there's almost nothing a kernel can do to protect from that. > Besides, the Windows OS resists against DMA attacks by disabling DMA of > external devices by default: > http://support.microsoft.com/kb/2516445 So does Linux :) > Considering that this patch is for a USB media driver, I think that an > attacker can just insert a malicious USB device to modify DMA memory and > trigger this bug. USB devices do not touch DMA memory so they physically can not do things like what thunderbolt devices can do. > Besides, not related to this patch, some drivers use DMA to send/receive > data (such as the URB used in USB drivers and ring descriptors used in > network drivers). In this case, if the data is malicious and used as an > array index through DMA, security problems may occur. > > In my opinion, similar to the data from userspace, the data from hardware > may be also malicious and should be checked. > > Maybe we could discuss this issue with DMA driver developers? Sure, but I think that's outside the scope of this tiny patch :) thanks, greg k-h
On 2020/5/6 19:07, Greg KH wrote: > On Wed, May 06, 2020 at 06:13:01PM +0800, Jia-Ju Bai wrote: >> I have never modified DMA memory in the real world, but an attacker can use >> a malicious device to do this. >> There is a video that shows how to use the Inception tool to perform DMA >> attacks and login in the Windows OS without password: >> https://www.youtube.com/watch?v=HDhpy7RpUjM > If you have control over the hardware, and can write to any DMA memory, > again, there's almost nothing a kernel can do to protect from that. I think that each device can only access its own DMA memory, instead of any DMA memory for other hardware devices. Thus, it is dangerous that the whole kernel can be attacked via a simple malicious device, through some vulnerabilities in the trusted driver. A feasible example is that, the attacker inserts a malicious device via PCI-E bus in a locked computer, when the owner of this computer leaves. The attacker modifies the DMA memory of this malicious device, to exploit the vulnerabilities in the trusted driver and steal key data from the computer. I think this is possible, because many buses (such PCI-E bus) support hot plugging now. Thus, to protect the kernel from such DMA attacks, these vulnerabilities in the trusted driver had better be fixed. Besides, I think that a safe way is to only store data in DMA memory, and this data is only passed to userspace and hardware, instead of tainting kernel data... >> Besides, not related to this patch, some drivers use DMA to send/receive >> data (such as the URB used in USB drivers and ring descriptors used in >> network drivers). In this case, if the data is malicious and used as an >> array index through DMA, security problems may occur. >> >> In my opinion, similar to the data from userspace, the data from hardware >> may be also malicious and should be checked. >> >> Maybe we could discuss this issue with DMA driver developers? > Sure, but I think that's outside the scope of this tiny patch :) Okay, in the discussion e-mail, I will introduce my opinion and give several possible DMA issues that I found, thanks :) Best wishes, Jia-Ju Bai
On Wed, May 06, 2020 at 11:30:22PM +0800, Jia-Ju Bai wrote: > > > On 2020/5/6 19:07, Greg KH wrote: > > On Wed, May 06, 2020 at 06:13:01PM +0800, Jia-Ju Bai wrote: > > > I have never modified DMA memory in the real world, but an attacker can use > > > a malicious device to do this. > > > There is a video that shows how to use the Inception tool to perform DMA > > > attacks and login in the Windows OS without password: > > > https://www.youtube.com/watch?v=HDhpy7RpUjM > > If you have control over the hardware, and can write to any DMA memory, > > again, there's almost nothing a kernel can do to protect from that. > > I think that each device can only access its own DMA memory, instead of any > DMA memory for other hardware devices. That's not true at all for all systems that Linux runs on. > Thus, it is dangerous that the whole kernel can be attacked via a simple > malicious device, through some vulnerabilities in the trusted driver. True, so restrict physical access. Or use a good iommu if you care about this :) > A feasible example is that, the attacker inserts a malicious device via > PCI-E bus in a locked computer, when the owner of this computer leaves. This is a semi-well-known issue. It's been described in the past regarding thunderbolt devices, and odds are, more people will run across it again in the future and also complain about it. The best solution is to solve this at the bus level, preventing different devices access to other memory areas. And providing physical access control to systems that you care about this type of attack for. Again, this isn't a new thing, but the ability for us to do much about it depends on the specific hardware control, and how we set defaults up. If you trust a device enough to plug it in, well, you need to trust it :) thanks, greg k-h
On 2020/5/6 23:52, Greg KH wrote: > On Wed, May 06, 2020 at 11:30:22PM +0800, Jia-Ju Bai wrote: >> >> On 2020/5/6 19:07, Greg KH wrote: >>> On Wed, May 06, 2020 at 06:13:01PM +0800, Jia-Ju Bai wrote: >>>> I have never modified DMA memory in the real world, but an attacker can use >>>> a malicious device to do this. >>>> There is a video that shows how to use the Inception tool to perform DMA >>>> attacks and login in the Windows OS without password: >>>> https://www.youtube.com/watch?v=HDhpy7RpUjM >>> If you have control over the hardware, and can write to any DMA memory, >>> again, there's almost nothing a kernel can do to protect from that. >> I think that each device can only access its own DMA memory, instead of any >> DMA memory for other hardware devices. > That's not true at all for all systems that Linux runs on. I am not sure to understand this. For example, a driver requests DMA memory with "len" size by using: mem = dma_alloc_coherent(..., len, ...); I think that the driver can only access DMA memory between "mem" and "mem + len", is it true? Can the driver access other DMA memory using some code like "mem + len * 10"? > >> A feasible example is that, the attacker inserts a malicious device via >> PCI-E bus in a locked computer, when the owner of this computer leaves. > This is a semi-well-known issue. It's been described in the past > regarding thunderbolt devices, and odds are, more people will run across > it again in the future and also complain about it. > > The best solution is to solve this at the bus level, preventing > different devices access to other memory areas. > > And providing physical access control to systems that you care about > this type of attack for. > > Again, this isn't a new thing, but the ability for us to do much about > it depends on the specific hardware control, and how we set defaults up. Yes, I agree that this issue is not new, because DMA attacks are old problems. But I am a little surprised that many current drivers are still vulnerable to DMA attacks. > > If you trust a device enough to plug it in, well, you need to trust it > :) Well, maybe I need to trust all devices in my computer :) Anyway, thanks a lot for your patient explanation and reply. If you have encountered other kinds of DMA-related bugs/vulnerabilities, maybe I can help to detect them using my static-analysis tool :) Best wishes, Jia-Ju Bai
On Thu, May 07, 2020 at 12:48:47AM +0800, Jia-Ju Bai wrote: > > > On 2020/5/6 23:52, Greg KH wrote: > > On Wed, May 06, 2020 at 11:30:22PM +0800, Jia-Ju Bai wrote: > > > > > > On 2020/5/6 19:07, Greg KH wrote: > > > > On Wed, May 06, 2020 at 06:13:01PM +0800, Jia-Ju Bai wrote: > > > > > I have never modified DMA memory in the real world, but an attacker can use > > > > > a malicious device to do this. > > > > > There is a video that shows how to use the Inception tool to perform DMA > > > > > attacks and login in the Windows OS without password: > > > > > https://www.youtube.com/watch?v=HDhpy7RpUjM > > > > If you have control over the hardware, and can write to any DMA memory, > > > > again, there's almost nothing a kernel can do to protect from that. > > > I think that each device can only access its own DMA memory, instead of any > > > DMA memory for other hardware devices. > > That's not true at all for all systems that Linux runs on. > > I am not sure to understand this. > For example, a driver requests DMA memory with "len" size by using: > mem = dma_alloc_coherent(..., len, ...); > I think that the driver can only access DMA memory between "mem" and "mem + > len", is it true? > Can the driver access other DMA memory using some code like "mem + len * > 10"? Depends on the hardware platform. > > > A feasible example is that, the attacker inserts a malicious device via > > > PCI-E bus in a locked computer, when the owner of this computer leaves. > > This is a semi-well-known issue. It's been described in the past > > regarding thunderbolt devices, and odds are, more people will run across > > it again in the future and also complain about it. > > > > The best solution is to solve this at the bus level, preventing > > different devices access to other memory areas. > > > > And providing physical access control to systems that you care about > > this type of attack for. > > > > Again, this isn't a new thing, but the ability for us to do much about > > it depends on the specific hardware control, and how we set defaults up. > > Yes, I agree that this issue is not new, because DMA attacks are old > problems. > But I am a little surprised that many current drivers are still vulnerable > to DMA attacks. Given that the attack vector is very hard to actually do, that's not a suprise. It's only a very recent thing that Linux drivers have started to work on "we don't trust the data coming from the hardware" path. Previously we always trusted that, but did not trust data coming from userspace. So work on fixing up drivers in this area is always encouraged. An example of this would be all of the fuzzing that USB drivers have been getting with custom loop-back interfaces and the like over the past year or so. Expanding that to "we don't trust PCI device data" should be the next step on this, and would help out your area as well. > > If you trust a device enough to plug it in, well, you need to trust it > > :) > > Well, maybe I need to trust all devices in my computer :) > > Anyway, thanks a lot for your patient explanation and reply. > If you have encountered other kinds of DMA-related bugs/vulnerabilities, > maybe I can help to detect them using my static-analysis tool :) Did you only find a problem in this one driver? Have you run it on any more "complex" drivers and gotten any good results showing either that we are programming defensively in this area, or not? thanks, greg k-h
On 2020/5/7 1:43, Greg KH wrote: > On Thu, May 07, 2020 at 12:48:47AM +0800, Jia-Ju Bai wrote: >> Yes, I agree that this issue is not new, because DMA attacks are old >> problems. >> But I am a little surprised that many current drivers are still vulnerable >> to DMA attacks. > Given that the attack vector is very hard to actually do, that's not > a suprise. > > It's only a very recent thing that Linux drivers have started to work on > "we don't trust the data coming from the hardware" path. Previously we > always trusted that, but did not trust data coming from userspace. So > work on fixing up drivers in this area is always encouraged. > > An example of this would be all of the fuzzing that USB drivers have > been getting with custom loop-back interfaces and the like over the past > year or so. Expanding that to "we don't trust PCI device data" should > be the next step on this, and would help out your area as well. Okay, I am glad to hear that :) Hardware security for the Linux kernel should receive more attention. Last year some researchers finished an interesting work about fuzzing the inputs from hardware: https://github.com/securesystemslab/periscope > >>> If you trust a device enough to plug it in, well, you need to trust it >>> :) >> Well, maybe I need to trust all devices in my computer :) >> >> Anyway, thanks a lot for your patient explanation and reply. >> If you have encountered other kinds of DMA-related bugs/vulnerabilities, >> maybe I can help to detect them using my static-analysis tool :) > Did you only find a problem in this one driver? Have you run it on any > more "complex" drivers and gotten any good results showing either that > we are programming defensively in this area, or not? > At present, I only detect the cases that a DMA value *directly* taints array index, loop condition and important kernel-interface calls (such as request_irq()). In this one driver, I only find two problems that mentioned in this patch. With the kernel configuration "allyesconfig" in my x86_64 machine, I find nearly 200 such problems (intra-procedurally and inter-procedurally) in all the compiled device drivers. I also find that several drivers check the data from DMA memory, but some of these checks can be bypassed. Here is an example in drivers/scsi/esas2r/esas2r_vda.c: The function esas2r_read_vda() uses a DMA value "vi": struct atto_ioctl_vda *vi = (struct atto_ioctl_vda *)a->vda_buffer; Then esas2r_read_vda() calls esas2r_process_vda_ioctl() with vi: esas2r_process_vda_ioctl(a, vi, rq, &sgc); In esas2r_process_vda_ioctl(), the DMA value "vi->function" is used at many places, such as: if (vi->function >= vercnt) ... if (vi->version > esas2r_vdaioctl_versions[vi->function]) ... However, when DMA failures or attacks occur, the value of vi->function can be changed at any time. In this case, vi->function can be first smaller than vercnt, and then it can be larger than vercnt when it is used as the array index of esas2r_vdaioctl_versions, causing a buffer-overflow vulnerability. I also submitted this patch, but no one has replied yet: https://lore.kernel.org/lkml/20200504172412.25985-1-baijiaju1990@gmail.com/ Best wishes, Jia-Ju Bai
On Thu, May 07, 2020 at 01:15:22PM +0800, Jia-Ju Bai wrote: > > > On 2020/5/7 1:43, Greg KH wrote: > > On Thu, May 07, 2020 at 12:48:47AM +0800, Jia-Ju Bai wrote: > > > Yes, I agree that this issue is not new, because DMA attacks are old > > > problems. > > > But I am a little surprised that many current drivers are still vulnerable > > > to DMA attacks. > > Given that the attack vector is very hard to actually do, that's not > > a suprise. > > > > It's only a very recent thing that Linux drivers have started to work on > > "we don't trust the data coming from the hardware" path. Previously we > > always trusted that, but did not trust data coming from userspace. So > > work on fixing up drivers in this area is always encouraged. > > > > An example of this would be all of the fuzzing that USB drivers have > > been getting with custom loop-back interfaces and the like over the past > > year or so. Expanding that to "we don't trust PCI device data" should > > be the next step on this, and would help out your area as well. > > Okay, I am glad to hear that :) > Hardware security for the Linux kernel should receive more attention. If you care about that, yes it should. At the least it is providing lots of graduate students with good research papers :) > Last year some researchers finished an interesting work about fuzzing the > inputs from hardware: > https://github.com/securesystemslab/periscope Nice! > > > > If you trust a device enough to plug it in, well, you need to trust it > > > > :) > > > Well, maybe I need to trust all devices in my computer :) > > > > > > Anyway, thanks a lot for your patient explanation and reply. > > > If you have encountered other kinds of DMA-related bugs/vulnerabilities, > > > maybe I can help to detect them using my static-analysis tool :) > > Did you only find a problem in this one driver? Have you run it on any > > more "complex" drivers and gotten any good results showing either that > > we are programming defensively in this area, or not? > > > > At present, I only detect the cases that a DMA value *directly* taints array > index, loop condition and important kernel-interface calls (such as > request_irq()). > In this one driver, I only find two problems that mentioned in this patch. > With the kernel configuration "allyesconfig" in my x86_64 machine, I find > nearly 200 such problems (intra-procedurally and inter-procedurally) in all > the compiled device drivers. > > I also find that several drivers check the data from DMA memory, but some of > these checks can be bypassed. > Here is an example in drivers/scsi/esas2r/esas2r_vda.c: > > The function esas2r_read_vda() uses a DMA value "vi": > struct atto_ioctl_vda *vi = > (struct atto_ioctl_vda *)a->vda_buffer; > > Then esas2r_read_vda() calls esas2r_process_vda_ioctl() with vi: > esas2r_process_vda_ioctl(a, vi, rq, &sgc); > > In esas2r_process_vda_ioctl(), the DMA value "vi->function" is > used at many places, such as: > if (vi->function >= vercnt) > ... > if (vi->version > esas2r_vdaioctl_versions[vi->function]) > ... > > However, when DMA failures or attacks occur, the value of vi->function can > be changed at any time. In this case, vi->function can be first smaller than > vercnt, and then it can be larger than vercnt when it is used as the array > index of esas2r_vdaioctl_versions, causing a buffer-overflow vulnerability. > > I also submitted this patch, but no one has replied yet: > https://lore.kernel.org/lkml/20200504172412.25985-1-baijiaju1990@gmail.com/ It's only been a few days, give them time. But, as with this patch, you might want to change your approach. Having the changelog say "this is a security problem!" really isn't that "real" as the threat model is very obscure at this point in time. Just say something like I referenced here, "read the value from memory and test it and use that value instead of constantly reading from memory each time in case it changes" is nicer and more realistic. It's a poential optimization as well, if the complier didn't already do it for us automatically (which you really should look into...) If you make up a large series of these, I'd be glad to take them through one of my trees to try to fix them all up at once, that's usually a simpler way to do cross-tree changes like this. thanks, greg k-h
On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote: > In ttusb_dec_init_usb(): > dec->irq_buffer = usb_alloc_coherent(...) > > Thus, "dec->irq_buffer" is a DMA value, and it is assigned to "buffer" > in ttusb_dec_handle_irq(): > char *buffer = dec->irq_buffer; > > When DMA failures or attacks occur, the value of buffer[4] can be > changed at any time. In this case, "buffer[4] - 1 < ARRAY_SIZE(rc_keys)" > can be first satisfied, and then the value of buffer[4] can be changed > to a large number, causing a buffer-overflow vulnerability. > > To avoid the risk of this vulnerability, buffer[4] is assigned to a > non-DMA local variable "index" at the beginning of > ttusb_dec_handle_irq(), and then this variable replaces each use of > buffer[4] in the function. This change is the urb completion handler. As I understand it, dma is done by the usb host controller, in this case in response to an interrupt request urb from the usb device side. In the completion handler, once we are done reading the data in the buffer, the urb is submitted again. Only then can the interrupt request urb be resent by the device, so there is no possibility of the dma buffer being overwritten. Now I might be mistaken of course. I can see that firewire and pci devices do suffer from the problems describe in the paper, but not usb. > Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> > --- > drivers/media/usb/ttusb-dec/ttusb_dec.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/ttusb-dec/ttusb_dec.c b/drivers/media/usb/ttusb-dec/ttusb_dec.c > index 3198f9624b7c..8543c552515b 100644 > --- a/drivers/media/usb/ttusb-dec/ttusb_dec.c > +++ b/drivers/media/usb/ttusb-dec/ttusb_dec.c > @@ -250,6 +250,7 @@ static void ttusb_dec_handle_irq( struct urb *urb) > struct ttusb_dec *dec = urb->context; > char *buffer = dec->irq_buffer; > int retval; > + u8 index = buffer[4]; > > switch(urb->status) { > case 0: /*success*/ > @@ -281,11 +282,11 @@ static void ttusb_dec_handle_irq( struct urb *urb) > * this should/could be added later ... > * for now lets report each signal as a key down and up > */ > - if (buffer[4] - 1 < ARRAY_SIZE(rc_keys)) { Here buffer[4] is signed char, so if buffer[4] == 0 then (buffer[4] - 1) = -1, this becomes "if (-1 < ARRAY_SIZE(rc_keys))", which evaluates to false, due to it becoming an unsigned compare. _I think_. Not the most readable code at all. > - dprintk("%s:rc signal:%d\n", __func__, buffer[4]); > - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 1); > + if (index - 1 < ARRAY_SIZE(rc_keys)) { > + dprintk("%s:rc signal:%d\n", __func__, index); > + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 1); > input_sync(dec->rc_input_dev); > - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 0); > + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 0); Like Greg said, this patch reduces the number of dereferences and makes the code much cleaner, but the commit message is misleading. > input_sync(dec->rc_input_dev); > } > } > -- > 2.17.1 Thanks, Sean
On 2020/5/7 15:52, Greg KH wrote: > On Thu, May 07, 2020 at 01:15:22PM +0800, Jia-Ju Bai wrote: >> At present, I only detect the cases that a DMA value *directly* taints array >> index, loop condition and important kernel-interface calls (such as >> request_irq()). >> In this one driver, I only find two problems that mentioned in this patch. >> With the kernel configuration "allyesconfig" in my x86_64 machine, I find >> nearly 200 such problems (intra-procedurally and inter-procedurally) in all >> the compiled device drivers. >> >> I also find that several drivers check the data from DMA memory, but some of >> these checks can be bypassed. >> Here is an example in drivers/scsi/esas2r/esas2r_vda.c: >> >> The function esas2r_read_vda() uses a DMA value "vi": >> struct atto_ioctl_vda *vi = >> (struct atto_ioctl_vda *)a->vda_buffer; >> >> Then esas2r_read_vda() calls esas2r_process_vda_ioctl() with vi: >> esas2r_process_vda_ioctl(a, vi, rq, &sgc); >> >> In esas2r_process_vda_ioctl(), the DMA value "vi->function" is >> used at many places, such as: >> if (vi->function >= vercnt) >> ... >> if (vi->version > esas2r_vdaioctl_versions[vi->function]) >> ... >> >> However, when DMA failures or attacks occur, the value of vi->function can >> be changed at any time. In this case, vi->function can be first smaller than >> vercnt, and then it can be larger than vercnt when it is used as the array >> index of esas2r_vdaioctl_versions, causing a buffer-overflow vulnerability. >> >> I also submitted this patch, but no one has replied yet: >> https://lore.kernel.org/lkml/20200504172412.25985-1-baijiaju1990@gmail.com/ > It's only been a few days, give them time. > > But, as with this patch, you might want to change your approach. Having > the changelog say "this is a security problem!" really isn't that "real" > as the threat model is very obscure at this point in time. > > Just say something like I referenced here, "read the value from memory > and test it and use that value instead of constantly reading from memory > each time in case it changes" is nicer and more realistic. It's a > poential optimization as well, if the complier didn't already do it for > us automatically (which you really should look into...) Okay, I used objdump to generate the assembly code of ttusb_dec.o (ttusb_dec.c is compiled with -O2). I found the following possible operations for "buffer[4] - 1" in the assembly code: ...... movsbl 0x4(%rbp), %r15d lea -0x1(%r15), %r13d cmp $0x19, %r13d ..... movsbl 0x4(%rbp), %r13d sub $0x1, %r13d ..... movsbl 0x4(%rbp), %ebp sub $0x1, %ebp ..... Thus, I guess that the compiler does not optimize these three accesses to "buffer[4] - 1". As you suggested, I will change my log and send a new patch, thanks :) > > If you make up a large series of these, I'd be glad to take them through > one of my trees to try to fix them all up at once, that's usually a > simpler way to do cross-tree changes like this. > Okay, I will organize my found issues, and send them to you. Thanks :) Best wishes, Jia-Ju Bai
Thanks for the reply, Sean :) On 2020/5/7 16:43, Sean Young wrote: > On Tue, May 05, 2020 at 10:21:10PM +0800, Jia-Ju Bai wrote: >> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> --- >> drivers/media/usb/ttusb-dec/ttusb_dec.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/usb/ttusb-dec/ttusb_dec.c b/drivers/media/usb/ttusb-dec/ttusb_dec.c >> index 3198f9624b7c..8543c552515b 100644 >> --- a/drivers/media/usb/ttusb-dec/ttusb_dec.c >> +++ b/drivers/media/usb/ttusb-dec/ttusb_dec.c >> @@ -250,6 +250,7 @@ static void ttusb_dec_handle_irq( struct urb *urb) >> struct ttusb_dec *dec = urb->context; >> char *buffer = dec->irq_buffer; >> int retval; >> + u8 index = buffer[4]; >> >> switch(urb->status) { >> case 0: /*success*/ >> @@ -281,11 +282,11 @@ static void ttusb_dec_handle_irq( struct urb *urb) >> * this should/could be added later ... >> * for now lets report each signal as a key down and up >> */ >> - if (buffer[4] - 1 < ARRAY_SIZE(rc_keys)) { > Here buffer[4] is signed char, so if buffer[4] == 0 then (buffer[4] - 1) = -1, > this becomes "if (-1 < ARRAY_SIZE(rc_keys))", which evaluates to false, > due to it becoming an unsigned compare. _I think_. I think you are right. Maybe I should use "int index = buffer[4]" here. >> - dprintk("%s:rc signal:%d\n", __func__, buffer[4]); >> - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 1); >> + if (index - 1 < ARRAY_SIZE(rc_keys)) { >> + dprintk("%s:rc signal:%d\n", __func__, index); >> + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 1); >> input_sync(dec->rc_input_dev); >> - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 0); >> + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 0); > Like Greg said, this patch reduces the number of dereferences and makes > the code much cleaner, but the commit message is misleading. Okay, I will change my log and send a new patch. Best wishes, Jia-Ju Bai
diff --git a/drivers/media/usb/ttusb-dec/ttusb_dec.c b/drivers/media/usb/ttusb-dec/ttusb_dec.c index 3198f9624b7c..8543c552515b 100644 --- a/drivers/media/usb/ttusb-dec/ttusb_dec.c +++ b/drivers/media/usb/ttusb-dec/ttusb_dec.c @@ -250,6 +250,7 @@ static void ttusb_dec_handle_irq( struct urb *urb) struct ttusb_dec *dec = urb->context; char *buffer = dec->irq_buffer; int retval; + u8 index = buffer[4]; switch(urb->status) { case 0: /*success*/ @@ -281,11 +282,11 @@ static void ttusb_dec_handle_irq( struct urb *urb) * this should/could be added later ... * for now lets report each signal as a key down and up */ - if (buffer[4] - 1 < ARRAY_SIZE(rc_keys)) { - dprintk("%s:rc signal:%d\n", __func__, buffer[4]); - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 1); + if (index - 1 < ARRAY_SIZE(rc_keys)) { + dprintk("%s:rc signal:%d\n", __func__, index); + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 1); input_sync(dec->rc_input_dev); - input_report_key(dec->rc_input_dev, rc_keys[buffer[4] - 1], 0); + input_report_key(dec->rc_input_dev, rc_keys[index - 1], 0); input_sync(dec->rc_input_dev); } }
In ttusb_dec_init_usb(): dec->irq_buffer = usb_alloc_coherent(...) Thus, "dec->irq_buffer" is a DMA value, and it is assigned to "buffer" in ttusb_dec_handle_irq(): char *buffer = dec->irq_buffer; When DMA failures or attacks occur, the value of buffer[4] can be changed at any time. In this case, "buffer[4] - 1 < ARRAY_SIZE(rc_keys)" can be first satisfied, and then the value of buffer[4] can be changed to a large number, causing a buffer-overflow vulnerability. To avoid the risk of this vulnerability, buffer[4] is assigned to a non-DMA local variable "index" at the beginning of ttusb_dec_handle_irq(), and then this variable replaces each use of buffer[4] in the function. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- drivers/media/usb/ttusb-dec/ttusb_dec.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)