On 28 Sep 2015 at 10:31, David Laight wrote:
> From: PaX Team
> > Sent: 26 September 2015 14:47
> > hi all,
> >
> > drivers/usb/host/r8a66597-hcd.c:r8a66597_hub_descriptor can truncate
> > r8a66597.max_root_hub (of type unsigned int) to an unsigned char:
> >
> > desc->bNbrPorts = r8a66597->max_root_hub;
> >
> > based on the surrounding code my guess is that max_root_hub can simply
> > be turned into an unsigned char field as it can't hold a value bigger
> > than U8_MAX. in fact its value can't be bigger than 7 anyway since
> > later the code uses it like this:
> >
> > desc->u.hs.DeviceRemovable[0] = ((1 << r8a66597->max_root_hub) - 1) << 1;
> >
> > can anyone tell me if this analysis is correct?
> >
> > FTR, this issue was detected with the upcoming version of the size overflow
> > plugin we have in PaX/grsecurity and there're a handful of similar cases in
> > the tree where potentially unwanted or unnecessary integer truncations occur,
> > this being one of these. any opinion/help is welcome!
>
> What do you intend doing to 'fix' these integer truncation warnings?
>
> IMHO adding C casts makes the code unreadable and can hide more
> serious errors.
there's no generic answer to this but the last thing we want to do is
add explicit casts (the size overflow plugin would see through them
anyway, so it's never the right answer for our purposes). for this
particular case what i described above should suffice if i'm not
mistaken:
--- linux-4.1.8/drivers/usb/host/r8a66597.h 2012-12-11 04:30:57.000000000 +0100
+++ linux-4.1.8-pax/drivers/usb/host/r8a66597.h 2015-09-26 15:38:21.774026439 +0200
@@ -125,7 +125,7 @@ struct r8a66597 {
unsigned short interval_map;
unsigned char pipe_cnt[R8A66597_MAX_NUM_PIPE];
unsigned char dma_map;
- unsigned int max_root_hub;
+ unsigned char max_root_hub;
struct list_head child_device;
unsigned long child_connect_map[4];
> If you want to detect invalid truncations you need a tool that
> tracks the domain of valid values for integer types.
the size overflow plugin doesn't detect truncations at compile time but
rather it instruments code to detect them (and other integer handling
related issues) at runtime. however when the instrumented code triggers
in some real life case (be that a false or true positive) we also look
at the underlying problem and for simple patterns like this we do a quick
static analysis on the whole tree to see how prevalent the issue is and
how hard it'd be to fix them at the source code level. the above case was
found by looking at stores to size related structure fields (that the
plugin had previously discovered) from other structure fields (e.g.,
from ->max_root_hub to ->bNbrPorts in the above example).
> It might need some source code annotation on the definition of fields.
the problem with annotations is that they don't scale (just look at the
lack of use of __user, etc), so we prefer automation above all else with
the occasional human help such as this case.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
