Re: [kbuild-all] [linuxtv-media:master 499/499] drivers/media/i2c/s5k5baf.c:362:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t'

2014-01-08 Thread Dan Carpenter 
The other thing that concerned me with this was the sparse warning:

drivers/media/i2c/s5k5baf.c:481:26: error: bad constant expression

It was hard to verify that this couldn't go over 512.  I guess 512 is
what we would consider an error in this context.  This seems like it
could be determined by the firmware?

regards,
dan carpenter

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kbuild-all] [linuxtv-media:master 499/499] drivers/media/i2c/s5k5baf.c:362:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t'

2014-01-08 Thread Mauro Carvalho Chehab 
Em Wed, 8 Jan 2014 11:37:37 +0300
Dan Carpenter dan.carpen...@oracle.com escreveu:

 The other thing that concerned me with this was the sparse warning:
 
 drivers/media/i2c/s5k5baf.c:481:26: error: bad constant expression

Hmm...
static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
  u16 count, const u16 *seq)
{
struct i2c_client *c = v4l2_get_subdevdata(state-sd);
__be16 buf[count + 1];
int ret, n;

Yeah, allocating data like that at stack is not nice.

I would simply replace the static allocation here by a dynamic one.
 
 It was hard to verify that this couldn't go over 512.  I guess 512 is
 what we would consider an error in this context.  This seems like it
 could be determined by the firmware?
 
 regards,
 dan carpenter
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-media in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kbuild-all] [linuxtv-media:master 499/499] drivers/media/i2c/s5k5baf.c:362:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t'

2014-01-08 Thread Andrzej Hajda 
On 01/08/2014 09:37 AM, Dan Carpenter wrote:
 The other thing that concerned me with this was the sparse warning:
 
 drivers/media/i2c/s5k5baf.c:481:26: error: bad constant expression
 
 It was hard to verify that this couldn't go over 512.  I guess 512 is
 what we would consider an error in this context.  This seems like it
 could be determined by the firmware?

Thanks for reporting.

I will prepare patch adding check for this.

Regards
Andrzej

 
 regards,
 dan carpenter
 

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kbuild-all] [linuxtv-media:master 499/499] drivers/media/i2c/s5k5baf.c:362:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t'

2014-01-08 Thread Andrzej Hajda 
On 01/08/2014 01:21 PM, Mauro Carvalho Chehab wrote:
 Em Wed, 8 Jan 2014 11:37:37 +0300
 Dan Carpenter dan.carpen...@oracle.com escreveu:

 The other thing that concerned me with this was the sparse warning:

 drivers/media/i2c/s5k5baf.c:481:26: error: bad constant expression
 Hmm...
   static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
 u16 count, const u16 *seq)
   {
   struct i2c_client *c = v4l2_get_subdevdata(state-sd);
   __be16 buf[count + 1];
   int ret, n;

 Yeah, allocating data like that at stack is not nice.

 I would simply replace the static allocation here by a dynamic one.
Sequences are very short (usually few words) and their length is known
in compile time.
The only exception are sequences provided by firmware file and for them
I can add check in s5k5baf_write_nseq to make it safe.

Replacing it with dynamic allocation seems to me unnecessary in this
particular case, it would result in memory allocation/free for every
single access to
the device. What do you think?

Regards
Andrzej

  
 It was hard to verify that this couldn't go over 512.  I guess 512 is
 what we would consider an error in this context.  This seems like it
 could be determined by the firmware?

 regards,
 dan carpenter

 --
 To unsubscribe from this list: send the line unsubscribe linux-media in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kbuild-all] [linuxtv-media:master 499/499] drivers/media/i2c/s5k5baf.c:362:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t'

2014-01-08 Thread Mauro Carvalho Chehab 
Em Wed, 08 Jan 2014 14:33:30 +0100
Andrzej Hajda a.ha...@samsung.com escreveu:

 On 01/08/2014 01:21 PM, Mauro Carvalho Chehab wrote:
  Em Wed, 8 Jan 2014 11:37:37 +0300
  Dan Carpenter dan.carpen...@oracle.com escreveu:
 
  The other thing that concerned me with this was the sparse warning:
 
  drivers/media/i2c/s5k5baf.c:481:26: error: bad constant expression
  Hmm...
  static void s5k5baf_write_arr_seq(struct s5k5baf *state, u16 addr,
u16 count, const u16 *seq)
  {
  struct i2c_client *c = v4l2_get_subdevdata(state-sd);
  __be16 buf[count + 1];
  int ret, n;
 
  Yeah, allocating data like that at stack is not nice.
 
  I would simply replace the static allocation here by a dynamic one.
 Sequences are very short (usually few words) and their length is known
 in compile time.
 The only exception are sequences provided by firmware file and for them
 I can add check in s5k5baf_write_nseq to make it safe.
 
 Replacing it with dynamic allocation seems to me unnecessary in this
 particular case, it would result in memory allocation/free for every
 single access to
 the device. What do you think?

Well, if you know in advance what's the maximum size, just replace it by
something like:

__be16 buf[64];

and check if count + 1 is less or equal to sizeof(buf).

As the kernel stack is really small, we should avoid allocating large
data there (1KB is large, on that sense).

Also, it would be possible o use i2cdev to inject very large payloads
to be sent to a random I2C device. That could cause a machine OOPS
or to use it to inject a security breach code.

So, we should really avoid using dynamic static allocation, specially
on I2C handlers.

Regards,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html