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

[Michael Zoran]

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.

#define MMAL_PARAMETER_CAMERA_INFO_MAX_CAMERAS 4
#define MMAL_PARAMETER_CAMERA_INFO_MAX_FLASHES 2
#define MMAL_PARAMETER_CAMERA_INFO_MAX_STR_LEN 16

struct mmal_parameter_camera_info_camera_t {
	u32    port_id;
	u32    max_width;
	u32    max_height;
	u32    lens_present;
	u8     camera_name[MMAL_PARAMETER_CAMERA_INFO_MAX_STR_LEN];
};

enum mmal_parameter_camera_info_flash_type_t {
	/* Make values explicit to ensure they match values in config
ini */
	MMAL_PARAMETER_CAMERA_INFO_FLASH_TYPE_XENON = 0,
	MMAL_PARAMETER_CAMERA_INFO_FLASH_TYPE_LED   = 1,
	MMAL_PARAMETER_CAMERA_INFO_FLASH_TYPE_OTHER = 2,
	MMAL_PARAMETER_CAMERA_INFO_FLASH_TYPE_MAX = 0x7FFFFFFF
};

struct mmal_parameter_camera_info_flash_t {
	enum mmal_parameter_camera_info_flash_type_t flash_type;
};

struct mmal_parameter_camera_info_t {
	u32                            num_cameras;
	u32                            num_flashes;
	struct mmal_parameter_camera_info_camera_t
				cameras[MMAL_PARAMETER_CAMERA_INFO_MAX_
CAMERAS];
	struct mmal_parameter_camera_info_flash_t
				flashes[MMAL_PARAMETER_CAMERA_INFO_MAX_
FLASHES];
};