| Message ID | d102a389-af14-45fd-a304-d2458c06cca3@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: fix 'read beyond size of field' | expand |
Hi Hans, On 9/26/23 11:27, Hans Verkuil wrote: > If CONFIG_FORTIFY_SOURCE=y, then this warning is produced: > > In file included from ./include/linux/string.h:254, > from ./include/linux/bitmap.h:11, > from ./include/linux/cpumask.h:12, > from ./arch/x86/include/asm/cpumask.h:5, > from ./arch/x86/include/asm/msr.h:11, > from ./arch/x86/include/asm/processor.h:23, > from ./arch/x86/include/asm/cpufeature.h:5, > from ./arch/x86/include/asm/thread_info.h:53, > from ./include/linux/thread_info.h:60, > from ./arch/x86/include/asm/preempt.h:9, > from ./include/linux/preempt.h:79, > from ./include/linux/spinlock.h:56, > from ./include/linux/mmzone.h:8, > from ./include/linux/gfp.h:7, > from ./include/linux/slab.h:16, > from ./drivers/staging/media/atomisp//include/hmm/hmm.h:26, > from drivers/staging/media/atomisp/pci/sh_css_params.c:26: > In function ‘fortify_memcpy_chk’, > inlined from ‘sh_css_store_sp_group_to_ddr’ at drivers/staging/media/atomisp/pci/sh_css_params.c:3736:3: > ./include/linux/fortify-string.h:592:25: warning: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? > [-Wattribute-warning] > 592 | __read_overflow2_field(q_size_field, size); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > The reason is that the memcpy copies two fields (each a u8), when the source > pointer points to the first field. It's a bit unexpected, so just make this > explicit. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > Hans, can you verify that it is indeed the intention of the original code > to write both bytes? Yes I believe so. I'm currently travelling so I cannot test this, but I think it should be fine, see feel free to merge this and if it breaks things I'll get back to you once I can actually run some tests. Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > > Note that I think that the initial memcpy is equally dubious. I think that > should be replaced by '*buf_ptr++ = ...' lines as well, rather than just > copying the first three fields with a memcpy. If you want I can make a v2 > that does that. > --- > diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c > index 5667e855da76..232744973ab8 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_params.c > +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c > @@ -3733,8 +3733,8 @@ ia_css_ptr sh_css_store_sp_group_to_ddr(void) > if (IS_ISP2401) { > memcpy(buf_ptr, &sh_css_sp_group.config, 3); > buf_ptr += 3; > - memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2); > - buf_ptr += 2; > + *buf_ptr++ = sh_css_sp_group.config.enable_isys_event_queue; > + *buf_ptr++ = sh_css_sp_group.config.disable_cont_vf; > memset(buf_ptr, 0, 3); > buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/ > } else { >
diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c index 5667e855da76..232744973ab8 100644 --- a/drivers/staging/media/atomisp/pci/sh_css_params.c +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c @@ -3733,8 +3733,8 @@ ia_css_ptr sh_css_store_sp_group_to_ddr(void) if (IS_ISP2401) { memcpy(buf_ptr, &sh_css_sp_group.config, 3); buf_ptr += 3; - memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2); - buf_ptr += 2; + *buf_ptr++ = sh_css_sp_group.config.enable_isys_event_queue; + *buf_ptr++ = sh_css_sp_group.config.disable_cont_vf; memset(buf_ptr, 0, 3); buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/ } else {
If CONFIG_FORTIFY_SOURCE=y, then this warning is produced: In file included from ./include/linux/string.h:254, from ./include/linux/bitmap.h:11, from ./include/linux/cpumask.h:12, from ./arch/x86/include/asm/cpumask.h:5, from ./arch/x86/include/asm/msr.h:11, from ./arch/x86/include/asm/processor.h:23, from ./arch/x86/include/asm/cpufeature.h:5, from ./arch/x86/include/asm/thread_info.h:53, from ./include/linux/thread_info.h:60, from ./arch/x86/include/asm/preempt.h:9, from ./include/linux/preempt.h:79, from ./include/linux/spinlock.h:56, from ./include/linux/mmzone.h:8, from ./include/linux/gfp.h:7, from ./include/linux/slab.h:16, from ./drivers/staging/media/atomisp//include/hmm/hmm.h:26, from drivers/staging/media/atomisp/pci/sh_css_params.c:26: In function ‘fortify_memcpy_chk’, inlined from ‘sh_css_store_sp_group_to_ddr’ at drivers/staging/media/atomisp/pci/sh_css_params.c:3736:3: ./include/linux/fortify-string.h:592:25: warning: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning] 592 | __read_overflow2_field(q_size_field, size); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The reason is that the memcpy copies two fields (each a u8), when the source pointer points to the first field. It's a bit unexpected, so just make this explicit. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- Hans, can you verify that it is indeed the intention of the original code to write both bytes? Note that I think that the initial memcpy is equally dubious. I think that should be replaced by '*buf_ptr++ = ...' lines as well, rather than just copying the first three fields with a memcpy. If you want I can make a v2 that does that. ---