Media: fix several issues on drivers
Subsystems (11)
cover Media: fix several issues on drivers
There are a number of issues that aren't passing on some static analyzer checks.
Address some of them.
Mauro Carvalho Chehab (13):
media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()
media: v4l2-tpg: prevent the risk of a division by zero
media: dvbdev: prevent the risk of out of memory access
media: dvb_frontend: don't play tricks with underflow values
media: mgb4: protect driver against spectre
media: av7110: fix a spectre vulnerability
media: s5p-jpeg: prevent buffer overflows
media: ar0521: don't overflow when checking PLL values
media: cx24116: prevent overflows on SNR calculus
media: adv7604 prevent underflow condition when reporting colorspace
media: stb0899_algo: initialize cfr before using it
media: cec: extron-da-hd-4k-plus: don't use -1 as an error code
media: pulse8-cec: fix data timestamp at pulse8_setup()
.../extron-da-hd-4k-plus.c | 6 ++--- drivers/media/cec/usb/pulse8/pulse8-cec.c | 2 +- drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++ drivers/media/dvb-core/dvb_frontend.c | 4 +-- drivers/media/dvb-core/dvbdev.c | 17 ++++++++++-- drivers/media/dvb-frontends/cx24116.c | 7 ++++- drivers/media/dvb-frontends/stb0899_algo.c | 2 +- drivers/media/i2c/adv7604.c | 26 ++++++++++++------- drivers/media/i2c/ar0521.c | 4 +-- drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ .../platform/samsung/s5p-jpeg/jpeg-core.c | 17 +++++++----- drivers/media/v4l2-core/v4l2-ctrls-api.c | 10 ++++--- drivers/staging/media/av7110/av7110.h | 4 ++- drivers/staging/media/av7110/av7110_ca.c | 25 ++++++++++++------ 14 files changed, 90 insertions(+), 39 deletions(-)
Signature · 1 line
2.47.0
PATCH 1/13 media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()
The error check logic at get_ctrl() is broken: if ptr_to_user() fails to fill a control due to an error, no errors are returned and v4l2_g_ctrl() returns success on a failed operation, which may cause applications to fail.
Add an error check at get_ctrl() and ensure that it will be returned to userspace without filling the control value if get_ctrl() fails.
--- drivers/media/v4l2-core/v4l2-ctrls-api.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls-api.c +7 -3
@@ -753,9 +753,10 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)for (i =0 ; i < master->ncontrols; i++)cur_to_new (master->cluster[i]); ret =call_op (master, g_volatile_ctrl); -new_to_user (c, ctrl); +if (!ret) + ret =new_to_user (c, ctrl); }else { -cur_to_user (c, ctrl); + ret =cur_to_user (c, ctrl); }v4l2_ctrl_unlock (master);return ret;
@@ -770,7 +771,10 @@ int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)if (!ctrl || !ctrl->is_int)return -EINVAL ; ret =get_ctrl (ctrl, &c); - control->value = c.value; + +if (!ret) + control->value = c.value; +return ret; }EXPORT_SYMBOL (v4l2_g_ctrl);
Signature · 1 line
2.47.0
Re: [PATCH 01/13] media: v4l2-ctrls-api: fix error handling for v4l2_g_ctrl()
Quoted 35 lines · Mauro Carvalho Chehab
The error check logic at
get_ctrl()is broken: ifptr_to_user()fails to fill a control due to an error, no errors are returned andv4l2_g_ctrl()returns success on a failed operation, which may cause applications to fail.Add an error check at
get_ctrl()and ensure that it will be returned to userspace without filling the control value ifget_ctrl()fails.Fixes 71c689dc2e73 ("media: v4l2-ctrls: split up into four source files")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/v4l2-core/v4l2-ctrls-api.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)drivers/media/v4l2-core/v4l2-ctrls-api.c +3
-2@@ -753,9 +753,10 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)for (i =0 ; i < master->ncontrols; i++)cur_to_new (master->cluster[i]); ret =call_op (master, g_volatile_ctrl); -new_to_user (c, ctrl); +if (!ret) + ret =new_to_user (c, ctrl); }else { -cur_to_user (c, ctrl); + ret =cur_to_user (c, ctrl); }v4l2_ctrl_unlock (master);return ret;@@ -770,7 +771,10 @@ int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)if (!ctrl || !ctrl->is_int)
drivers/media/v4l2-core/v4l2-ctrls-api.c int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control) · line 780
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls-api.c +4
-1return -EINVAL ; ret =get_ctrl (ctrl, &c); - control->value = c.value; + +if (!ret) + control->value = c.value; +return ret; }EXPORT_SYMBOL (v4l2_g_ctrl);
Yeah, that's better.
There are also unchecked calls to cur_to_user() and user_to_new() in set_ctrl_lock().
Can you fix that as well?
Regards,
Hans
PATCH 2/13 media: v4l2-tpg: prevent the risk of a division by zero
In reply to Mauro Carvalho ChehabThe logic at tpg_precalculate_line() blindly rescales the buffer even when scaled_witdh is equal to zero. If this ever happens, this will cause a division by zero.
Instead, add a WARN_ON() to trigger such cases and return without doing any precalculation.
--- drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +3 -0
@@ -1795,6 +1795,9 @@ static void tpg_precalculate_line(struct tpg_data *tpg)unsigned p;unsigned x; +if (WARN_ON (tpg->src_width ==0 )) +return ; +switch (tpg->pattern) {case TPG_PAT_GREEN : contrast =TPG_COLOR_100_RED ;
Signature · 1 line
2.47.0
Re: [PATCH 02/13] media: v4l2-tpg: prevent the risk of a division by zero
Quoted 2 lines · Mauro Carvalho Chehab
The logic at
tpg_precalculate_line()blindly rescales the buffer even whenscaled_witdhis equal to zero. If this ever
scaled_witdh -> scaled_width
Quoted 17 lines
happens, this will cause a division by zero.
Instead, add a
WARN_ON()to trigger such cases and return without doing any precalculation.Fixes 63881df94d3e ("[media] vivid: add the Test Pattern Generator")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++ 1 file changed, 3 insertions(+)drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +0
-0
drivers/media/common/v4l2-tpg/v4l2-tpg-core.c static void tpg_precalculate_line(struct tpg_data *tpg) · line 1798
Quoted 4 lines
drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +1
-0@@ -1795,6 +1795,9 @@ static void tpg_precalculate_line(struct tpg_data *tpg)unsigned p;unsigned x; +if (WARN_ON (tpg->src_width ==0 ))
You need to check for both src_width and scaled_width.
Also replace this by WARN_ON_ONCE.
Regards,
Hans
Quoted 5 lines
drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +2
-0+return ; +switch (tpg->pattern) {case TPG_PAT_GREEN : contrast =TPG_COLOR_100_RED ;
PATCH 3/13 media: dvbdev: prevent the risk of out of memory access
In reply to Mauro Carvalho ChehabThe dvbdev contains a static variable used to store dvb minors.
The behavior of it depends if CONFIG_DVB_DYNAMIC_MINORS is set or not. When not set, dvb_register_device() won't check for boundaries, as it will rely that a previous call to dvb_register_adapter() would already be enforcing it.
On a similar way, dvb_device_open() uses the assumption that the register functions already did the needed checks.
This can be fragile if some device ends using different calls. This also generate warnings on static check analysers.
So, add explicit guards to prevent potential risk of OOM issues.
--- drivers/media/dvb-core/dvbdev.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvbdev.c +15 -2
@@ -86,10 +86,15 @@ static DECLARE_RWSEM(minor_rwsem);static int dvb_device_open (struct inode *inode,struct file *file) {struct dvb_device *dvbdev; +unsigned int minor =iminor (inode); + +if (minor >=MAX_DVB_MINORS ) +return -ENODEV ;mutex_lock (&dvbdev_mutex);down_read (&minor_rwsem); - dvbdev = dvb_minors[iminor (inode)]; + + dvbdev = dvb_minors[minor];if (dvbdev && dvbdev->fops) {int err =0 ;
@@ -525,7 +530,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,for (minor =0 ; minor <MAX_DVB_MINORS ; minor++)if (!dvb_minors[minor])break ; -if (minor ==MAX_DVB_MINORS ) { +if (minor >=MAX_DVB_MINORS ) {if (new_node) {list_del (&new_node->list_head);kfree (dvbdevfops);
@@ -540,6 +545,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, }#else minor =nums2minor (adap->num, type, id); +if (minor >=MAX_DVB_MINORS ) { +dvb_media_device_free (dvbdev); +list_del (&dvbdev->list_head); +kfree (dvbdev); + *pdvbdev =NULL ; +mutex_unlock (&dvbdev_register_lock); +return ret; + }#endif dvbdev->minor = minor; dvb_minors[minor] =dvb_device_get (dvbdev);
Signature · 1 line
2.47.0
PATCH 4/13 media: dvb_frontend: don't play tricks with underflow values
In reply to Mauro Carvalho Chehabfepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue.
It relies to have this code just afterwards:
if (!ready) fepriv->auto_sub_step++;
Simplify the code by simply setting it to zero and use continue to return to the while loop.
--- drivers/media/dvb-core/dvb_frontend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/dvb-core/dvb_frontend.c +2 -2
@@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct dvb_frontend *fe, int check_wradefault : fepriv->auto_step++; - fepriv->auto_sub_step =-1 ;/* it'll be incremented to 0 in a moment */ -break ; + fepriv->auto_sub_step =0 ; +continue ; }if (!ready) fepriv->auto_sub_step++;
Signature · 1 line
2.47.0
PATCH 5/13 media: mgb4: protect driver against spectre
In reply to Mauro Carvalho ChehabFrequency range is set from sysfs via frequency_range_store(), being vulnerable to spectre, as reported by smatch:
drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r] drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half. 'reg_set'
Fix it.
--- drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/media/pci/mgb4/mgb4_cmt.c +2 -0
@@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,u32 config;size_t i; + freq_range =array_index_nospec (freq_range,ARRAY_SIZE (cmt_vals_in)); + addr = cmt_addrs_in[vindev->config->id]; reg_set = cmt_vals_in[freq_range];
Signature · 1 line
2.47.0
Re: [PATCH 05/13] media: mgb4: protect driver against spectre
Quoted 20 lines · Mauro Carvalho Chehab
Frequency range is set from sysfs via
frequency_range_store(), being vulnerable to spectre, as reported by smatch:
drivers/media/pci/mgb4/mgb4_cmt.c:231mgb4_cmt_set_vin_freq_range()warn: potential spectre issue 'cmt_vals_in' [r]drivers/media/pci/mgb4/mgb4_cmt.c:238mgb4_cmt_set_vin_freq_range()warn: possible spectre second half. 'reg_set'Fix it.
Fixes 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ 1 file changed, 2 insertions(+)drivers/media/pci/mgb4/mgb4_cmt.c +0
-0
drivers/media/pci/mgb4/mgb4_cmt.c void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, · line 234
Quoted 8 lines
drivers/media/pci/mgb4/mgb4_cmt.c +2
-0@@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,u32 config;size_t i; + freq_range =array_index_nospec (freq_range,ARRAY_SIZE (cmt_vals_in)); + addr = cmt_addrs_in[vindev->config->id]; reg_set = cmt_vals_in[freq_range];
I still do not fully understand the exact vulnerability here, but the patch should definitely not do any harm, so I'm ok with it even if it's real purpose would only be to silence the smatch warning :-)
Re: [PATCH 05/13] media: mgb4: protect driver against spectre
Em Wed, 16 Oct 2024 13:59:18 +0200
Martin Tůma <[email protected]> escreveu:
Quoted 24 lines · Mauro Carvalho Chehab
Quoted 23 lines
Frequency range is set from sysfs via
frequency_range_store(), being vulnerable to spectre, as reported by smatch:
drivers/media/pci/mgb4/mgb4_cmt.c:231mgb4_cmt_set_vin_freq_range()warn: potential spectre issue 'cmt_vals_in' [r]drivers/media/pci/mgb4/mgb4_cmt.c:238mgb4_cmt_set_vin_freq_range()warn: possible spectre second half. 'reg_set'Fix it.
Fixes 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ 1 file changed, 2 insertions(+)drivers/media/pci/mgb4/mgb4_cmt.c +0
-0@@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,u32 config;size_t i;
Quoted 9 lines
drivers/media/pci/mgb4/mgb4_cmt.c void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, · line 234
Quoted 5 lines
drivers/media/pci/mgb4/mgb4_cmt.c +2
-0+ freq_range =array_index_nospec (freq_range,ARRAY_SIZE (cmt_vals_in)); + addr = cmt_addrs_in[vindev->config->id]; reg_set = cmt_vals_in[freq_range];I still do not fully understand the exact vulnerability here, but the patch should definitely not do any harm, so I'm ok with it even if it's real purpose would only be to silence the smatch warning :-)
With Spectre, just checking if freq_range is between 0 and the size of the array is not enough, as malicious code could use CPU speculative logic to retrieve data from memory outside the limits of the array.
As freq_range is specified by the user via sysfs attribute frequency_range, it is subject to Spectre v1 attack as described at Documentation/admin-guide/hw-vuln/spectre.rst.
Silencing smatch is a plus.
Quoted 2 lines
Reviewed-by Martin Tůma <[email protected]>
Thanks!
Thanks,
Mauro
Re: [PATCH 05/13] media: mgb4: protect driver against spectre
Hi,
Quoted 37 lines · Mauro Carvalho Chehab
Em Wed, 16 Oct 2024 13:59:18 +0200
Martin Tůma <[email protected]> escreveu:Quoted 24 lines · Mauro Carvalho Chehab
Quoted 23 lines
Frequency range is set from sysfs via
frequency_range_store(), being vulnerable to spectre, as reported by smatch:
drivers/media/pci/mgb4/mgb4_cmt.c:231mgb4_cmt_set_vin_freq_range()warn: potential spectre issue 'cmt_vals_in' [r]drivers/media/pci/mgb4/mgb4_cmt.c:238mgb4_cmt_set_vin_freq_range()warn: possible spectre second half. 'reg_set'Fix it.
Fixes 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ 1 file changed, 2 insertions(+)drivers/media/pci/mgb4/mgb4_cmt.c +0
-0@@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev,u32 config;size_t i;Quoted 9 lines
drivers/media/pci/mgb4/mgb4_cmt.c void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, · line 234
Quoted 5 lines
drivers/media/pci/mgb4/mgb4_cmt.c +2
-0+ freq_range =array_index_nospec (freq_range,ARRAY_SIZE (cmt_vals_in)); + addr = cmt_addrs_in[vindev->config->id]; reg_set = cmt_vals_in[freq_range];I still do not fully understand the exact vulnerability here, but the patch should definitely not do any harm, so I'm ok with it even if it's real purpose would only be to silence the smatch warning :-)
Quoted 9 lines
With Spectre, just checking if
freq_rangeis between 0 and the size of the array is not enough, as malicious code could use CPU speculative logic to retrieve data from memory outside the limits of the array.As
freq_rangeis specified by the user via sysfs attributefrequency_range, it is subject to Spectre v1 attack as described atDocumentation/admin-guide/hw-vuln/spectre.rst.
I do know the general theory about the "spectre bounds check fix/workaround", what I was referring is this part in the documentation:
"Such speculative memory accesses can leave side effects, creating side channels which leak information to the attacker."
I do not see/understand the exact "information leak" that could happen here on this particular place. But as already stated in the original answer, I don't have to understand everything ;-)
M.
Quoted 10 lines
Silencing smatch is a plus.
Quoted 2 lines
Reviewed-by Martin Tůma <[email protected]>Thanks!
Thanks,
Mauro
PATCH 6/13 media: av7110: fix a spectre vulnerability
In reply to Mauro Carvalho ChehabAs warned by smatch:
drivers/staging/media/av7110/av7110_ca.c:270 dvb_ca_ioctl() warn: potential spectre issue 'av7110->ci_slot' [w] (local cap)
There is a spectre-related vulnerability at the code. Fix it.
--- drivers/staging/media/av7110/av7110.h | 4 +++- drivers/staging/media/av7110/av7110_ca.c | 25 ++++++++++++++++-------- 2 files changed, 20 insertions(+), 9 deletions(-)
drivers/staging/media/av7110/av7110.h +3 -1
@@ -88,6 +88,8 @@ struct infrared {u32 ir_config; }; +#define MAX_CI_SLOTS 2 +/* place to store all the necessary device information */ struct av7110 {/* devices */
@@ -163,7 +165,7 @@ struct av7110 {/* CA */ -struct ca_slot_info ci_slot[2 ]; +struct ca_slot_info ci_slot[MAX_CI_SLOTS ];enum av7110_video_mode vidmode;struct dmxdev dmxdev;
drivers/staging/media/av7110/av7110_ca.c +17 -8
@@ -26,23 +26,28 @@void CI_handle (struct av7110 *av7110,u8 *data,u16 len) { +unsigned slot_num; +dprintk (8 ,"av7110:%p\n" , av7110);if (len <3 )return ;switch (data[0 ]) {case CI_MSG_CI_INFO : -if (data[2 ] !=1 && data[2 ] !=2 ) +if (data[2 ] !=1 && data[2 ] !=MAX_CI_SLOTS )break ; + + slot_num =array_index_nospec (data[2 ] -1 ,MAX_CI_SLOTS ); +switch (data[1 ]) {case 0 : - av7110->ci_slot[data[2 ] -1 ].flags =0 ; + av7110->ci_slot[slot_num].flags =0 ;break ;case 1 : - av7110->ci_slot[data[2 ] -1 ].flags |=CA_CI_MODULE_PRESENT ; + av7110->ci_slot[slot_num].flags |=CA_CI_MODULE_PRESENT ;break ;case 2 : - av7110->ci_slot[data[2 ] -1 ].flags |=CA_CI_MODULE_READY ; + av7110->ci_slot[slot_num].flags |=CA_CI_MODULE_READY ;break ; }break ;
@@ -262,15 +267,19 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)case CA_GET_SLOT_INFO : {struct ca_slot_info *info = (struct ca_slot_info *)parg; +unsigned int slot_num;if (info->num <0 || info->num >1 ) {mutex_unlock (&av7110->ioctl_mutex);return -EINVAL ; } - av7110->ci_slot[info->num].num = info->num; - av7110->ci_slot[info->num].type =FW_CI_LL_SUPPORT (av7110->arm_app) ? -CA_CI_LINK :CA_CI ; -memcpy (info, &av7110->ci_slot[info->num],sizeof (struct ca_slot_info )); + slot_num =array_index_nospec (info->num,MAX_CI_SLOTS ); + + av7110->ci_slot[slot_num].num = info->num; + av7110->ci_slot[slot_num].type =FW_CI_LL_SUPPORT (av7110->arm_app) ? +CA_CI_LINK :CA_CI ; +memcpy (info, &av7110->ci_slot[slot_num], +sizeof (struct ca_slot_info ));break ; }
Signature · 1 line
2.47.0
PATCH 7/13 media: s5p-jpeg: prevent buffer overflows
In reply to Mauro Carvalho ChehabThe current logic allows word to be less than 2. If this happens, there will be buffer overflows. Add extra checks to prevent it.
While here, remove an unused word = 0 assignment.
--- .../media/platform/samsung/s5p-jpeg/jpeg-core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c +11 -6
@@ -775,11 +775,14 @@ static void exynos4_jpeg_parse_decode_h_tbl(struct s5p_jpeg_ctx *ctx) (unsigned long )vb2_plane_vaddr (&vb->vb2_buf,0 ) + ctx->out_q.sos +2 ; jpeg_buffer.curr =0 ; - word =0 ; -if (get_word_be (&jpeg_buffer, &word))return ; - jpeg_buffer.size = (long )word -2 ; + +if (word <2 ) + jpeg_buffer.size =0 ; +else + jpeg_buffer.size = (long )word -2 ; + jpeg_buffer.data +=2 ; jpeg_buffer.curr =0 ;
@@ -1058,6 +1061,7 @@ static int get_word_be(struct s5p_jpeg_buffer *buf, unsigned int *word)if (byte ==-1 )return -1 ; *word = (unsigned int )byte | temp; +return 0 ; }
@@ -1145,7 +1149,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; -if (!length) +if (length <=0 )return false; sof = jpeg_buffer.curr;/* after 0xffc0 */ sof_len = length;
@@ -1176,7 +1180,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; -if (!length) +if (length <=0 )return false;if (n_dqt >=S5P_JPEG_MAX_MARKER )return false;
@@ -1189,7 +1193,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; -if (!length) +if (length <=0 )return false;if (n_dht >=S5P_JPEG_MAX_MARKER )return false;
@@ -1214,6 +1218,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; +/* No need to check underflows as skip() does it */ skip (&jpeg_buffer, length);break ; }
Signature · 1 line
2.47.0
Re: [PATCH 07/13] media: s5p-jpeg: prevent buffer overflows
Hi Mauro,
Quoted 70 lines · Mauro Carvalho Chehab
The current logic allows word to be less than 2. If this happens, there will be buffer overflows. Add extra checks to prevent it.
While here, remove an unused word = 0 assignment.
Fixes 6c96dbbc2aa9 ("[media] s5p-jpeg: add support for 5433")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- .../media/platform/samsung/s5p-jpeg/jpeg-core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c +10
-6@@ -775,11 +775,14 @@ static void exynos4_jpeg_parse_decode_h_tbl(struct s5p_jpeg_ctx *ctx) (unsigned long )vb2_plane_vaddr (&vb->vb2_buf,0 ) + ctx->out_q.sos +2 ; jpeg_buffer.curr =0 ; - word =0 ; -if (get_word_be (&jpeg_buffer, &word))return ; - jpeg_buffer.size = (long )word -2 ; + +if (word <2 ) + jpeg_buffer.size =0 ; +else + jpeg_buffer.size = (long )word -2 ; + jpeg_buffer.data +=2 ; jpeg_buffer.curr =0 ;@@ -1058,6 +1061,7 @@ static int get_word_be(struct s5p_jpeg_buffer *buf, unsigned int *word)if (byte ==-1 )return -1 ; *word = (unsigned int )byte | temp; +return 0 ; }@@ -1145,7 +1149,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; -if (!length) +if (length <=0 )return false; sof = jpeg_buffer.curr;/* after 0xffc0 */ sof_len = length;@@ -1176,7 +1180,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; -if (!length) +if (length <=0 )return false;if (n_dqt >=S5P_JPEG_MAX_MARKER )return false;@@ -1189,7 +1193,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; -if (!length) +if (length <=0 )return false;if (n_dht >=S5P_JPEG_MAX_MARKER )return false;
drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result, · line 1224
Quoted 7 lines
drivers/media/platform/samsung/s5p-jpeg/jpeg-core.c +1
-0@@ -1214,6 +1218,7 @@ static bool s5p_jpeg_parse_hdr(struct s5p_jpeg_q_data *result,if (get_word_be (&jpeg_buffer, &word))break ; length = (long )word -2 ; +/* No need to check underflows as skip() does it */ skip (&jpeg_buffer, length);break ; }
Seems reasonable.
Signature · 2 lines
Best regards, Jacek Anaszewski
PATCH 8/13 media: ar0521: don't overflow when checking PLL values
In reply to Mauro Carvalho ChehabThe PLL checks are comparing 64 bit integers with 32 bit ones. Depending on the values of the variables, this may underflow.
Fix it ensuring that both sides of the expression are u64.
--- drivers/media/i2c/ar0521.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/i2c/ar0521.c +2 -2
@@ -255,10 +255,10 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *multcontinue ;/* Minimum value */ if (new_mult >254 )break ;/* Maximum, larger pre won't work either */ -if (sensor->extclk_freq * (u64 )new_mult <AR0521_PLL_MIN * +if (sensor->extclk_freq * (u64 )new_mult < (u64 )AR0521_PLL_MIN * new_pre)continue ; -if (sensor->extclk_freq * (u64 )new_mult >AR0521_PLL_MAX * +if (sensor->extclk_freq * (u64 )new_mult > (u64 )AR0521_PLL_MAX * new_pre)break ;/* Larger pre won't work either */ new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult,
Signature · 1 line
2.47.0
Re: [PATCH 08/13] media: ar0521: don't overflow when checking PLL values
Hi Mauro,
Quoted 20 lines · Mauro Carvalho Chehab
The PLL checks are comparing 64 bit integers with 32 bit ones. Depending on the values of the variables, this may underflow.
Fix it ensuring that both sides of the expression are u64.
Fixes 852b50aeed15 ("media: On Semi AR0521 sensor driver")Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/i2c/ar0521.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/media/i2c/ar0521.c +0
-0@@ -255,10 +255,10 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *multcontinue ;/* Minimum value */ if (new_mult >254 )
drivers/media/i2c/ar0521.c static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult · line 264
Quoted 10 lines
drivers/media/i2c/ar0521.c +2
-2break ;/* Maximum, larger pre won't work either */ -if (sensor->extclk_freq * (u64 )new_mult <AR0521_PLL_MIN * +if (sensor->extclk_freq * (u64 )new_mult < (u64 )AR0521_PLL_MIN * new_pre)continue ; -if (sensor->extclk_freq * (u64 )new_mult >AR0521_PLL_MAX * +if (sensor->extclk_freq * (u64 )new_mult > (u64 )AR0521_PLL_MAX * new_pre)break ;/* Larger pre won't work either */ new_pll = div64_round_up(sensor->extclk_freq * (u64)new_mult,
Signature · 1 line
Sakari Ailus
PATCH 9/13 media: cx24116: prevent overflows on SNR calculus
In reply to Mauro Carvalho ChehabIf reading SNR registers fail, a negative number will be returned, causing an underflow when reading SNR registers.
Prevent that.
--- drivers/media/dvb-frontends/cx24116.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/media/dvb-frontends/cx24116.c +6 -1
@@ -741,6 +741,7 @@ static int cx24116_read_snr_pct(struct dvb_frontend *fe, u16 *snr) {struct cx24116_state *state = fe->demodulator_priv;u8 snr_reading; +int ret;static const u32 snr_tab[] = {/* 10 x Table (rounded up) */ 0x00000 ,0x0199A ,0x03333 ,0x04ccD ,0x06667 ,0x08000 ,0x0999A ,0x0b333 ,0x0cccD ,0x0e667 ,
@@ -749,7 +750,11 @@ static int cx24116_read_snr_pct(struct dvb_frontend *fe, u16 *snr)dprintk ("%s()\n" , __func__); - snr_reading =cx24116_readreg (state,CX24116_REG_QUALITY0 ); + ret =cx24116_readreg (state,CX24116_REG_QUALITY0 ); +if (ret <0 ) +return ret; + + snr_reading = ret;if (snr_reading >=0xa0 /* 100% */ ) *snr =0xffff ;
Signature · 1 line
2.47.0
PATCH 10/13 media: adv7604 prevent underflow condition when reporting colorspace
In reply to Mauro Carvalho ChehabCurrently, adv76xx_log_status() reads some date using io_read() which may return negative values. The current logi doesn't check such errors, causing colorspace to be reported on a wrong way at adv76xx_log_status().
If I/O error happens there, print a different message, instead of reporting bogus messages to userspace.
--- drivers/media/i2c/adv7604.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)
drivers/media/i2c/adv7604.c +17 -9
@@ -2519,10 +2519,10 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)const struct adv76xx_chip_info *info = state->info;struct v4l2_dv_timings timings;struct stdi_readback stdi; -u8 reg_io_0x02 =io_read (sd,0x02 ); +int ret; +u8 reg_io_0x02;u8 edid_enabled;u8 cable_det; -static const char *const csc_coeff_sel_rb[16 ] = {"bypassed" ,"YPbPr601 -> RGB" ,"reserved" ,"YPbPr709 -> RGB" ,"reserved" ,"RGB -> YPbPr601" ,"reserved" ,"RGB -> YPbPr709" ,
@@ -2621,13 +2621,21 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)v4l2_info (sd,"-----Color space-----\n" );v4l2_info (sd,"RGB quantization range ctrl: %s\n" , rgb_quantization_range_txt[state->rgb_quantization_range]); -v4l2_info (sd,"Input color space: %s\n" , - input_color_space_txt[reg_io_0x02 >>4 ]); -v4l2_info (sd,"Output color space: %s %s, alt-gamma %s\n" , - (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , - (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? -"(16-235)" :"(0-255)" , - (reg_io_0x02 &0x08 ) ?"enabled" :"disabled" ); + + ret =io_read (sd,0x02 ); +if (ret <0 ) { +v4l2_info (sd,"Can't read Input/Output color space\n" ); + }else { + reg_io_0x02 = ret; + +v4l2_info (sd,"Input color space: %s\n" , + input_color_space_txt[reg_io_0x02 >>4 ]); +v4l2_info (sd,"Output color space: %s %s, alt-gamma %s\n" , + (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , + (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? +"(16-235)" :"(0-255)" , + (reg_io_0x02 &0x08 ) ?"enabled" :"disabled" ); + }v4l2_info (sd,"Color space conversion: %s\n" , csc_coeff_sel_rb[cp_read (sd, info->cp_csc) >>4 ]);
Signature · 1 line
2.47.0
Re: [PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace
Quoted 10 lines · Mauro Carvalho Chehab
Currently,
adv76xx_log_status()reads some date usingio_read()which may return negative values. The current logi doesn't check such errors, causing colorspace to be reported on a wrong way atadv76xx_log_status().If I/O error happens there, print a different message, instead of reporting bogus messages to userspace.
Fixes 54450f591c99 ("[media] adv7604: driver for the Analog Devices ADV7604 video decoder")
Not really a fix since this would just affect logging for debugging purposes. I would personally just drop the Fixes and Cc tag.
Quoted 1 line
Signed-off-by Mauro Carvalho Chehab <[email protected]>
Regards,
Hans
Quoted 50 lines
--- drivers/media/i2c/adv7604.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)drivers/media/i2c/adv7604.c +17
-9@@ -2519,10 +2519,10 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)const struct adv76xx_chip_info *info = state->info;struct v4l2_dv_timings timings;struct stdi_readback stdi; -u8 reg_io_0x02 =io_read (sd,0x02 ); +int ret; +u8 reg_io_0x02;u8 edid_enabled;u8 cable_det; -static const char *const csc_coeff_sel_rb[16 ] = {"bypassed" ,"YPbPr601 -> RGB" ,"reserved" ,"YPbPr709 -> RGB" ,"reserved" ,"RGB -> YPbPr601" ,"reserved" ,"RGB -> YPbPr709" ,@@ -2621,13 +2621,21 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)v4l2_info (sd,"-----Color space-----\n" );v4l2_info (sd,"RGB quantization range ctrl: %s\n" , rgb_quantization_range_txt[state->rgb_quantization_range]); -v4l2_info (sd,"Input color space: %s\n" , - input_color_space_txt[reg_io_0x02 >>4 ]); -v4l2_info (sd,"Output color space: %s %s, alt-gamma %s\n" , - (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , - (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? -"(16-235)" :"(0-255)" , - (reg_io_0x02 &0x08 ) ?"enabled" :"disabled" ); + + ret =io_read (sd,0x02 ); +if (ret <0 ) { +v4l2_info (sd,"Can't read Input/Output color space\n" ); + }else { + reg_io_0x02 = ret; + +v4l2_info (sd,"Input color space: %s\n" , + input_color_space_txt[reg_io_0x02 >>4 ]); +v4l2_info (sd,"Output color space: %s %s, alt-gamma %s\n" , + (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , + (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? +"(16-235)" :"(0-255)" , + (reg_io_0x02 &0x08 ) ?"enabled" :"disabled" ); + }v4l2_info (sd,"Color space conversion: %s\n" , csc_coeff_sel_rb[cp_read (sd, info->cp_csc) >>4 ]);
Re: [PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace
Em Wed, 16 Oct 2024 12:57:53 +0200
Hans Verkuil <[email protected]> escreveu:
Quoted 6 lines · Mauro Carvalho Chehab
Quoted 5 lines
Currently,
adv76xx_log_status()reads some date usingio_read()which may return negative values. The current logi doesn't check such errors, causing colorspace to be reported on a wrong way atadv76xx_log_status().
Quoted 8 lines
Quoted 5 lines
If I/O error happens there, print a different message, instead of reporting bogus messages to userspace.
Fixes 54450f591c99 ("[media] adv7604: driver for the Analog Devices ADV7604 video decoder")Not really a fix since this would just affect logging for debugging purposes. I would personally just drop the Fixes and Cc tag.
The issue is on a VIDIOC_ ioctl, so part of media API. Ok, this is used only for debugging purposes and should, instead be implemented via debugfs, etc, but, in summary: it is what it is: part of the V4L2 uAPI.
-
Now, the question about what should have Fixes: tag and what shouldn't is a different matter. I've saw long discussions about that at the kernel mailing lists. In the particular case of y2038, I'm pretty sure I saw some of them with Fixes tag on it.
Quoted 51 lines
Quoted 2 lines
Signed-off-by Mauro Carvalho Chehab <[email protected]>Reviewed-by Hans Verkuil <[email protected]>Regards,
Hans
Quoted 42 lines
--- drivers/media/i2c/adv7604.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)drivers/media/i2c/adv7604.c +12
-9@@ -2519,10 +2519,10 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)const struct adv76xx_chip_info *info = state->info;struct v4l2_dv_timings timings;struct stdi_readback stdi; -u8 reg_io_0x02 =io_read (sd,0x02 ); +int ret; +u8 reg_io_0x02;u8 edid_enabled;u8 cable_det; -static const char *const csc_coeff_sel_rb[16 ] = {"bypassed" ,"YPbPr601 -> RGB" ,"reserved" ,"YPbPr709 -> RGB" ,"reserved" ,"RGB -> YPbPr601" ,"reserved" ,"RGB -> YPbPr709" ,@@ -2621,13 +2621,21 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)v4l2_info (sd,"-----Color space-----\n" );v4l2_info (sd,"RGB quantization range ctrl: %s\n" , rgb_quantization_range_txt[state->rgb_quantization_range]); -v4l2_info (sd,"Input color space: %s\n" , - input_color_space_txt[reg_io_0x02 >>4 ]); -v4l2_info (sd,"Output color space: %s %s, alt-gamma %s\n" , - (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , - (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? -"(16-235)" :"(0-255)" , - (reg_io_0x02 &0x08 ) ?"enabled" :"disabled" ); + + ret =io_read (sd,0x02 ); +if (ret <0 ) { +v4l2_info (sd,"Can't read Input/Output color space\n" ); + }else { + reg_io_0x02 = ret; + +v4l2_info (sd,"Input color space: %s\n" , + input_color_space_txt[reg_io_0x02 >>4 ]); + v4l2_info(sd, "Output color space: %s %s, alt-gamma %s\n",
Quoted 9 lines
drivers/media/i2c/adv7604.c static int adv76xx_log_status(struct v4l2_subdev *sd) · line 2641
Quoted 8 lines
drivers/media/i2c/adv7604.c +5
-0+ (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , + (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? +"(16-235)" :"(0-255)" , + (reg_io_0x02 & 0x08) ? "enabled" : "disabled"); + }v4l2_info (sd,"Color space conversion: %s\n" , csc_coeff_sel_rb[cp_read (sd, info->cp_csc) >>4 ]);
Thanks,
Mauro
Re: [PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace
Quoted 15 lines · Mauro Carvalho Chehab
Em Wed, 16 Oct 2024 12:57:53 +0200
Hans Verkuil <[email protected]> escreveu:Quoted 12 lines · Mauro Carvalho Chehab
Quoted 10 lines
Currently,
adv76xx_log_status()reads some date usingio_read()which may return negative values. The current logi doesn't check such errors, causing colorspace to be reported on a wrong way atadv76xx_log_status().If I/O error happens there, print a different message, instead of reporting bogus messages to userspace.
Fixes 54450f591c99 ("[media] adv7604: driver for the Analog Devices ADV7604 video decoder")
Quoted 7 lines
Quoted 2 lines
Not really a fix since this would just affect logging for debugging purposes. I would personally just drop the Fixes and Cc tag.
The issue is on a
VIDIOC_ioctl, so part of media API. Ok, this is used only for debugging purposes and should, instead be implemented via debugfs, etc, but, in summary: it is what it is: part of the V4L2 uAPI.
The ioctl, yes, but what it logs to the kernel log isn't part of the ABI. That can change.
I think it is overkill to send this to stable for an old chip that almost nobody uses, and that requires an i2c read to go wrong for it to produce a wrong debug message. It seems an unnecessary waste of time.
Quoted 7 lines
-
Now, the question about what should have Fixes: tag and what shouldn't is a different matter. I've saw long discussions about that at the kernel mailing lists. In the particular case of y2038, I'm pretty sure I saw some of them with Fixes tag on it.
But patch 13/13 doesn't affect the operation either, again it is just an incorrect log message that can only go wrong if Pulse-Eight still sells that device in 2038 with a firmware build date >= 2038. And v6.12 is guaranteed to be EOL in 2038 :-)
Regards,
Hans
Quoted 66 lines
Quoted 52 lines
Quoted 3 lines
Signed-off-by Mauro Carvalho Chehab <[email protected]>Reviewed-by Hans Verkuil <[email protected]>Regards,
Hans
Quoted 42 lines
--- drivers/media/i2c/adv7604.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)drivers/media/i2c/adv7604.c +12
-9@@ -2519,10 +2519,10 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)const struct adv76xx_chip_info *info = state->info;struct v4l2_dv_timings timings;struct stdi_readback stdi; -u8 reg_io_0x02 =io_read (sd,0x02 ); +int ret; +u8 reg_io_0x02;u8 edid_enabled;u8 cable_det; -static const char *const csc_coeff_sel_rb[16 ] = {"bypassed" ,"YPbPr601 -> RGB" ,"reserved" ,"YPbPr709 -> RGB" ,"reserved" ,"RGB -> YPbPr601" ,"reserved" ,"RGB -> YPbPr709" ,@@ -2621,13 +2621,21 @@ static int adv76xx_log_status(struct v4l2_subdev *sd)v4l2_info (sd,"-----Color space-----\n" );v4l2_info (sd,"RGB quantization range ctrl: %s\n" , rgb_quantization_range_txt[state->rgb_quantization_range]); -v4l2_info (sd,"Input color space: %s\n" , - input_color_space_txt[reg_io_0x02 >>4 ]); -v4l2_info (sd,"Output color space: %s %s, alt-gamma %s\n" , - (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , - (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? -"(16-235)" :"(0-255)" , - (reg_io_0x02 &0x08 ) ?"enabled" :"disabled" ); + + ret =io_read (sd,0x02 ); +if (ret <0 ) { +v4l2_info (sd,"Can't read Input/Output color space\n" ); + }else { + reg_io_0x02 = ret; + +v4l2_info (sd,"Input color space: %s\n" , + input_color_space_txt[reg_io_0x02 >>4 ]); + v4l2_info(sd, "Output color space: %s %s, alt-gamma %s\n",Quoted 9 lines
drivers/media/i2c/adv7604.c static int adv76xx_log_status(struct v4l2_subdev *sd) · line 2641
Quoted 8 lines
drivers/media/i2c/adv7604.c +5
-0+ (reg_io_0x02 &0x02 ) ?"RGB" :"YCbCr" , + (((reg_io_0x02 >>2 ) &0x01 ) ^ (reg_io_0x02 &0x01 )) ? +"(16-235)" :"(0-255)" , + (reg_io_0x02 & 0x08) ? "enabled" : "disabled"); + }v4l2_info (sd,"Color space conversion: %s\n" , csc_coeff_sel_rb[cp_read (sd, info->cp_csc) >>4 ]);Thanks,
Mauro
Re: [PATCH 10/13] media: adv7604 prevent underflow condition when reporting colorspace
Em Wed, 16 Oct 2024 13:58:48 +0200
Hans Verkuil <[email protected]> escreveu:
Quoted 19 lines · Mauro Carvalho Chehab
Quoted 18 lines
Em Wed, 16 Oct 2024 12:57:53 +0200
Hans Verkuil <[email protected]> escreveu:Quoted 14 lines · Mauro Carvalho Chehab
Quoted 10 lines
Currently,
adv76xx_log_status()reads some date usingio_read()which may return negative values. The current logi doesn't check such errors, causing colorspace to be reported on a wrong way atadv76xx_log_status().If I/O error happens there, print a different message, instead of reporting bogus messages to userspace.
Fixes 54450f591c99 ("[media] adv7604: driver for the Analog Devices ADV7604 video decoder")Not really a fix since this would just affect logging for debugging purposes. I would personally just drop the Fixes and Cc tag.
Quoted 7 lines
Quoted 4 lines
The issue is on a
VIDIOC_ioctl, so part of media API. Ok, this is used only for debugging purposes and should, instead be implemented via debugfs, etc, but, in summary: it is what it is: part of the V4L2 uAPI.The ioctl, yes, but what it logs to the kernel log isn't part of the ABI. That can change.
Sure, logs can change, but this is an user-visible bug.
Quoted 3 lines
I think it is overkill to send this to stable for an old chip that almost nobody uses, and that requires an i2c read to go wrong for it to produce a wrong debug message. It seems an unnecessary waste of time.
Agreed. Will drop cc stable.
Quoted 8 lines
Quoted 7 lines
-
Now, the question about what should have Fixes: tag and what shouldn't is a different matter. I've saw long discussions about that at the kernel mailing lists. In the particular case of y2038, I'm pretty sure I saw some of them with Fixes tag on it.
Quoted 5 lines
But patch 13/13 doesn't affect the operation either, again it is just an incorrect log message that can only go wrong if Pulse-Eight still sells that device in 2038 with a firmware build date >= 2038.
And v6.12 is guaranteed to be EOL in 2038 :-)
We can't count on it. Civil infrastructure is now working with a 10 years SLTS:
I heard somewhere that having a 15 years or 20 years stable Kernel is a need for certain usages.
Even commercial distros have a minimum of 10 years as LTS.
Suse is now working with a 13-years support. Both Canonical and Red Hat announced a 12-years ELTS support. As they usually takes the last year's LTS Kernel, it means support will end with a 14 years old Kernel (so, support will end in 2037 or 2038 if they release an LTS distro next year), and don't decide to extend it further.
I also heard during LPC that there's an increased pressure from Linux customers from commercial distros to extend it even further.
Thanks,
Mauro
PATCH 11/13 media: stb0899_algo: initialize cfr before using it
In reply to Mauro Carvalho ChehabThe loop at stb0899_search_carrier() starts with a random value for cfr. Initialize it to zero, just like stb0899_dvbs_algo() to ensure that carrier search won't bail out.
--- drivers/media/dvb-frontends/stb0899_algo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/dvb-frontends/stb0899_algo.c +1 -1
@@ -269,7 +269,7 @@ static enum stb0899_status stb0899_search_carrier(struct stb0899_state *state)short int derot_freq =0 , last_derot_freq =0 , derot_limit, next_loop =3 ;int index =0 ; -u8 cfr[2 ]; +u8 cfr[2 ] = {0 };u8 reg; internal->status =NOCARRIER ;
Signature · 1 line
2.47.0
PATCH 12/13 media: cec: extron-da-hd-4k-plus: don't use -1 as an error code
In reply to Mauro Carvalho ChehabThe logic at get_edid_tag_location() returns either an offset or an error condition. However, the error condition uses a non-standard "-1" value.
Use instead -ENOENT to indicate that the tag was not found.
--- .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c +3 -3
@@ -348,12 +348,12 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,/* Return if not a CTA-861 extension block */ if (size <256 || edid[0 ] !=0x02 || edid[1 ] !=0x03 ) -return -1 ; +return -ENOENT ;/* search tag */ d = edid[0x02 ] &0x7f ;if (d <=4 ) -return -1 ; +return -ENOENT ; i =0x04 ; end =0x00 + d;
@@ -371,7 +371,7 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,return offset + i; i += len +1 ; }while (i < end); -return -1 ; +return -ENOENT ; }static void extron_edid_crc (u8 *edid)
Signature · 1 line
2.47.0
Re: [PATCH 12/13] media: cec: extron-da-hd-4k-plus: don't use -1 as an error code
Quoted 7 lines · Mauro Carvalho Chehab
The logic at
get_edid_tag_location()returns either an offset or an error condition. However, the error condition uses a non-standard "-1" value.Use instead
-ENOENTto indicate that the tag was not found.Fixes 056f2821b631 ("media: cec: extron-da-hd-4k-plus: add the Extron DA HD 4K Plus CEC driver")
Not a fix, since it isn't broken. It is returning an offset, and -1 is used if it isn't found. It's fine to change it to -ENOENT since the code calling it just checks if the result > 0.
So it is a slight improvement of the code, but not a fix.
Quoted 1 line
So this isn't needed either.
Regards,
Hans
Quoted 33 lines
Signed-off-by Mauro Carvalho Chehab <[email protected]>--- .../cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/media/cec/usb/extron-da-hd-4k-plus/extron-da-hd-4k-plus.c +3
-3@@ -348,12 +348,12 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,/* Return if not a CTA-861 extension block */ if (size <256 || edid[0 ] !=0x02 || edid[1 ] !=0x03 ) -return -1 ; +return -ENOENT ;/* search tag */ d = edid[0x02 ] &0x7f ;if (d <=4 ) -return -1 ; +return -ENOENT ; i =0x04 ; end =0x00 + d;@@ -371,7 +371,7 @@ static int get_edid_tag_location(const u8 *edid, unsigned int size,return offset + i; i += len +1 ; }while (i < end); -return -1 ; +return -ENOENT ; }static void extron_edid_crc (u8 *edid)
PATCH 13/13 media: pulse8-cec: fix data timestamp at pulse8_setup()
In reply to Mauro Carvalho ChehabThere is a hidden overflow condition there. As date is signed and u8 is unsigned, doing:
date = (data[0] << 24)
With a value bigger than 07f will make all upper bits of date 0xffffffff. This can be demonstrated with this small code:
<code>
typedef int64_t time64_t;
typedef uint8_t u8;
int main(void)
{
u8 data[] = { 0xde ,0xad , 0xbe, 0xef };
time64_t date;
date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
printf("Invalid data = 0x%08lx\n", date);
date = ((unsigned)data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
printf("Expected data = 0x%08lx\n", date);
return 0;
}</code>
Fix it by converting the upper bit calculation to unsigned.
--- drivers/media/cec/usb/pulse8/pulse8-cec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/cec/usb/pulse8/pulse8-cec.c +1 -1
@@ -685,7 +685,7 @@ static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio, err =pulse8_send_and_wait (pulse8, cmd,1 , cmd[0 ],4 );if (err)return err; - date = (data[0 ] <<24 ) | (data[1 ] <<16 ) | (data[2 ] <<8 ) | data[3 ]; + date = ((unsigned )data[0 ] <<24 ) | (data[1 ] <<16 ) | (data[2 ] <<8 ) | data[3 ];dev_info (pulse8->dev,"Firmware build date %ptT\n" , &date);dev_dbg (pulse8->dev,"Persistent config:\n" );
Signature · 1 line
2.47.0
Re: [PATCH 13/13] media: pulse8-cec: fix data timestamp at pulse8_setup()
Quoted 26 lines · Mauro Carvalho Chehab
There is a hidden overflow condition there. As date is signed and u8 is unsigned, doing:
date = (data[0] << 24)With a value bigger than 07f will make all upper bits of date
0xffffffff. This can be demonstrated with this small code:<code>
typedef
int64_ttime64_t;
typedefuint8_tu8;int main(void)
{ u8 data[] = { 0xde ,0xad , 0xbe, 0xef }; time64_t date; date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3]; printf("Invalid data = 0x%08lx\n", date); date = ((unsigned)data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3]; printf("Expected data = 0x%08lx\n", date); return 0; }
Quoted 6 lines
</code>
Fix it by converting the upper bit calculation to unsigned.
Fixes cea28e7a55e7 ("media: pulse8-cec: reorganize function order")
Not a fix either, just an improvement. The worst that can happen is that in 2038 the wrong date is shown, provided they release new firmware for this device in that year :-)
Regards,
Hans
Quoted 18 lines
Signed-off-by Mauro Carvalho Chehab <[email protected]>--- drivers/media/cec/usb/pulse8/pulse8-cec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/media/cec/usb/pulse8/pulse8-cec.c +1
-1@@ -685,7 +685,7 @@ static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio, err =pulse8_send_and_wait (pulse8, cmd,1 , cmd[0 ],4 );if (err)return err; - date = (data[0 ] <<24 ) | (data[1 ] <<16 ) | (data[2 ] <<8 ) | data[3 ]; + date = ((unsigned )data[0 ] <<24 ) | (data[1 ] <<16 ) | (data[2 ] <<8 ) | data[3 ];dev_info (pulse8->dev,"Firmware build date %ptT\n" , &date);dev_dbg (pulse8->dev,"Persistent config:\n" );