I've added the smatch mailing list to the CC list.
Joe had a question about some warnings. Please, don't do work arounds
for bugs in smatch. It's smatch which should be fixed.
These are the warnings:
drivers/scsi/mpt2sas/mpt2sas_ctl.c:683 _ctl_do_mpt_command() error: copy_from_user() 'mpi_request' too small (65535 vs 4294967292)
drivers/scsi/mpt2sas/mpt2sas_ctl.c:683 _ctl_do_mpt_command() error: copy_from_user() 'mpi_request' too small (65535 vs 4294967292)
drivers/scsi/mpt2sas/mpt2sas_ctl.c:713 _ctl_do_mpt_command() error: memcpy() 'mpi_request' too small (65535 vs 4294967292)
The code looks something like this:
mpi_request = kzalloc(ioc->request_sz, GFP_KERNEL)
[ snip ]
/* Check for overflow and wraparound */
if (karg.data_sge_offset * 4 > ioc->request_sz ||
karg.data_sge_offset > (UINT_MAX / 4)) {
ret = -EINVAL;
goto out;
}
[ snip ]
copy_from_user(mpi_request, mf, karg.data_sge_offset*4)
What's happening is that smatch mistakenly thinks the wrap around test
is a limit test so "karg.data_sge_offset" can go up to UINT_MAX / 4.
"io->request_sz" is unsigned short so it only goes up to USHRT_MAX.
UINT_MAX is more than USHRT_MAX so it's a buffer overflow.
But, of course, the real limit is "karg.data_sge_offset * 4 >
ioc->request_sz".
There are a couple ways to fix this and eventually smatch will try to
do both.
First we need to record that "buf_size(mpi_request) == ioc->request_sz".
There isn't any code to record these relationships currently. Next
smatch_comparison.c needs to record that
"karg.data_sge_offset * 4 <= ioc->request_sz". The code only handles
direct variable comparisons like "a <= b" and not "a * 4 <= b" but that
is something which can be fixed. Then, if we have these two
relationships, we can know that the copy doesn't overflow.
The other thing is that when we hit the
"karg.data_sge_offset * 4 > ioc->request_sz", we could move the 4 to
the other side which gives "karg.data_sge_offset > ioc->request_sz / 4".
So "karg.data_sge_offset is 0 to (USHRT_MAX / 4). Knowing that would
also eliminate the warning. That would be a change to smatch_extra.c.
Currently smatch tries to move all the known values in a comparison to
one side, but it could try moving them here as well.
But it's going to take some months before the code to do this is
finished.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe smatch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
