[PATCH 08/10] staging: bcm2835-camera: Fix buffer overflow calculation on query of camera properties

[Dave Stevenson]

On 10 March 2017 at 11:16, Michael Zoran <mzoran at crowfest.net> wrote:
> On Fri, 2017-03-10 at 11:11 +0000, Dave Stevenson wrote:
>> On 10 March 2017 at 05:08, Michael Zoran <mzoran at crowfest.net> wrote:
>> > The code that queries properties on the camera has a check
>> > for buffer overruns if the firmware sends too much data.  This
>> > check is incorrect, and during testing I was seeing stack
>> > corruption.
>> >
>> > I believe this error can actually happen in normal use, just for
>> > some reason it doesn't appear on 32 bit as often.  So perhaps
>> > it's best for the check to be fixed.
>>
>> That sounds like it is related to a report I couldn't nail down as it
>> only happened on ArchLinux -
>> https://github.com/raspberrypi/linux/issues/1447
>>
>> Comparing against the original MMAL implementation at
>> https://github.com/raspberrypi/userland/blob/master/interface/mmal/vc
>> /mmal_vc_api.c#L1187
>> there are a few subtle differences. Your patch is a belt and braces
>> to
>> ensure that it never copies too much, so reasonable in that regard.
>>
>> I'm actually more worried that it implies we have a structure
>> mismatch
>> against the firmware, but I currently can't see where it is. A load
>> of
>> debug prints for sizeof(struct) required....
>>
>> > Signed-off-by: Michael Zoran <mzoran at crowfest.net>
>> > ---
>> >  drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/staging/vc04_services/bcm2835-camera/mmal-
>> > vchiq.c b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
>> > index 41de8956e421..976aa08365f2 100644
>> > --- a/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
>> > +++ b/drivers/staging/vc04_services/bcm2835-camera/mmal-vchiq.c
>> > @@ -1442,7 +1442,7 @@ static int port_parameter_get(struct
>> > vchiq_mmal_instance *instance,
>> >         }
>> >
>> >         ret = -rmsg->u.port_parameter_get_reply.status;
>> > -       if (ret) {
>> > +       if (ret || (rmsg->u.port_parameter_get_reply.size >
>> > *value_size)) {
>> >                 /* Copy only as much as we have space for
>> >                  * but report true size of parameter
>> >                  */
>> > --
>> > 2.11.0
>> >
>
> I actually tracked it while debugging down to these structures.  I was
> going to let you know, but I went with the band-aid for now.
>
> The firmware thinks the structure should be 8 bytes longer.  I'm
> wondering if it should be 4 cameras and 4 flashes. That would explain
> the extra 8 bytes.

The userland repo is the extraction of the relevant IPC files from the
firmware source tree. What you see there is the same code the firmware
is being built from. It's only a 32bit compiler so I can't see why it
would suddenly be aligning the flash structures to 64bits.

I'll do some debugging on the firmware side to work out how/where it
is going wrong, but this patch is valid regardless.