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@xxxxxxxxx/
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
