* [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() @ 2025-12-11 6:36 Tuo Li 2025-12-11 19:18 ` Scott Branden 0 siblings, 1 reply; 7+ messages in thread From: Tuo Li @ 2025-12-11 6:36 UTC (permalink / raw) To: scott.branden, bcm-kernel-feedback-list, arnd, gregkh Cc: linux-kernel, Tuo Li In the function bcm_vk_read(), the pointer entry is checked, indicating that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the following code may cause null-pointer dereferences: struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; set_msg_id(&tmp_msg, entry->usr_msg_id); tmp_msg.size = entry->to_h_blks - 1; To prevent these possible null-pointer dereferences, copy to_h_msg, usr_msg_id, and to_h_blks from iter into temporary variables, and return these temporary variables to the application instead of accessing them through a potentially NULL entry. Signed-off-by: Tuo Li <islituo@gmail.com> --- drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c index 1f42d1d5a630..665a3888708a 100644 --- a/drivers/misc/bcm-vk/bcm_vk_msg.c +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, struct device *dev = &vk->pdev->dev; struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; struct bcm_vk_wkent *entry = NULL, *iter; + struct vk_msg_blk tmp_msg; + u32 tmp_usr_msg_id; + u32 tmp_blks; u32 q_num; u32 rsp_length; @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, entry = iter; } else { /* buffer not big enough */ + tmp_msg = iter->to_h_msg[0]; + tmp_usr_msg_id = iter->usr_msg_id; + tmp_blks = iter->to_h_blks; rc = -EMSGSIZE; } goto read_loop_exit; @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, bcm_vk_free_wkent(dev, entry); } else if (rc == -EMSGSIZE) { - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; - /* * in this case, return just the first block, so * that app knows what size it is looking for. */ - set_msg_id(&tmp_msg, entry->usr_msg_id); - tmp_msg.size = entry->to_h_blks - 1; + set_msg_id(&tmp_msg, tmp_usr_msg_id); + tmp_msg.size = tmp_blks - 1; if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); rc = -EFAULT; -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() 2025-12-11 6:36 [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() Tuo Li @ 2025-12-11 19:18 ` Scott Branden 2025-12-11 23:42 ` Tuo Li 0 siblings, 1 reply; 7+ messages in thread From: Scott Branden @ 2025-12-11 19:18 UTC (permalink / raw) To: Tuo Li; +Cc: bcm-kernel-feedback-list, arnd, gregkh, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2967 bytes --] Hi Tuo, On Wed, Dec 10, 2025 at 10:36 PM Tuo Li <islituo@gmail.com> wrote: > > In the function bcm_vk_read(), the pointer entry is checked, indicating > that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the > following code may cause null-pointer dereferences: > > struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > set_msg_id(&tmp_msg, entry->usr_msg_id); > tmp_msg.size = entry->to_h_blks - 1; > > To prevent these possible null-pointer dereferences, copy to_h_msg, > usr_msg_id, and to_h_blks from iter into temporary variables, and return > these temporary variables to the application instead of accessing them > through a potentially NULL entry. > > Signed-off-by: Tuo Li <islituo@gmail.com> > --- > drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c > index 1f42d1d5a630..665a3888708a 100644 > --- a/drivers/misc/bcm-vk/bcm_vk_msg.c > +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c > @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, > struct device *dev = &vk->pdev->dev; > struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; > struct bcm_vk_wkent *entry = NULL, *iter; > + struct vk_msg_blk tmp_msg; > + u32 tmp_usr_msg_id; > + u32 tmp_blks; > u32 q_num; > u32 rsp_length; > > @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, > entry = iter; > } else { > /* buffer not big enough */ > + tmp_msg = iter->to_h_msg[0]; > + tmp_usr_msg_id = iter->usr_msg_id; > + tmp_blks = iter->to_h_blks; > rc = -EMSGSIZE; > } > goto read_loop_exit; > @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, > > bcm_vk_free_wkent(dev, entry); > } else if (rc == -EMSGSIZE) { > - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > - > /* > * in this case, return just the first block, so > * that app knows what size it is looking for. > */ > - set_msg_id(&tmp_msg, entry->usr_msg_id); > - tmp_msg.size = entry->to_h_blks - 1; You can just change entry to iter on these 2 lines here? > + set_msg_id(&tmp_msg, tmp_usr_msg_id); > + tmp_msg.size = tmp_blks - 1; > if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { > dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); > rc = -EFAULT; > -- > 2.43.0 > [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 5473 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() 2025-12-11 19:18 ` Scott Branden @ 2025-12-11 23:42 ` Tuo Li 2025-12-11 23:59 ` Scott Branden 0 siblings, 1 reply; 7+ messages in thread From: Tuo Li @ 2025-12-11 23:42 UTC (permalink / raw) To: Scott Branden; +Cc: bcm-kernel-feedback-list, arnd, gregkh, linux-kernel Hi Scott, Thanks for your reply! On Fri, Dec 12, 2025 at 3:18 AM Scott Branden <scott.branden@broadcom.com> wrote: > > Hi Tuo, > > On Wed, Dec 10, 2025 at 10:36 PM Tuo Li <islituo@gmail.com> wrote: > > > > In the function bcm_vk_read(), the pointer entry is checked, indicating > > that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the > > following code may cause null-pointer dereferences: > > > > struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > set_msg_id(&tmp_msg, entry->usr_msg_id); > > tmp_msg.size = entry->to_h_blks - 1; > > > > To prevent these possible null-pointer dereferences, copy to_h_msg, > > usr_msg_id, and to_h_blks from iter into temporary variables, and return > > these temporary variables to the application instead of accessing them > > through a potentially NULL entry. > > > > Signed-off-by: Tuo Li <islituo@gmail.com> > > --- > > drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c > > index 1f42d1d5a630..665a3888708a 100644 > > --- a/drivers/misc/bcm-vk/bcm_vk_msg.c > > +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c > > @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > struct device *dev = &vk->pdev->dev; > > struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; > > struct bcm_vk_wkent *entry = NULL, *iter; > > + struct vk_msg_blk tmp_msg; > > + u32 tmp_usr_msg_id; > > + u32 tmp_blks; > > u32 q_num; > > u32 rsp_length; > > > > @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > entry = iter; > > } else { > > /* buffer not big enough */ > > + tmp_msg = iter->to_h_msg[0]; > > + tmp_usr_msg_id = iter->usr_msg_id; > > + tmp_blks = iter->to_h_blks; > > rc = -EMSGSIZE; > > } > > goto read_loop_exit; > > @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > bcm_vk_free_wkent(dev, entry); > > } else if (rc == -EMSGSIZE) { > > - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > - > > /* > > * in this case, return just the first block, so > > * that app knows what size it is looking for. > > */ > > - set_msg_id(&tmp_msg, entry->usr_msg_id); > > - tmp_msg.size = entry->to_h_blks - 1; > You can just change entry to iter on these 2 lines here? > > > + set_msg_id(&tmp_msg, tmp_usr_msg_id); > > + tmp_msg.size = tmp_blks - 1; > > if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { > > dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); > > rc = -EFAULT; > > -- > > 2.43.0 > > Thanks for the suggestion! My understanding is that you are suggesting replacing entry with iter in the rc == -EMSGSIZE path. However, I am not sure whether the objects referenced by iter can be modified or freed by other threads, since this path runs outside the protection of chan->pendq lock. If that can happen, accessing the objects via iter may lead to a data race or a use-after-free. Could you please help confirm whether this scenario is possible? If my concern is unfounded, I will prepare a v2 patch and replace entry with iter as you suggested. Thanks again for your time and helpful feedback! Sincerely, Tuo Li ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() 2025-12-11 23:42 ` Tuo Li @ 2025-12-11 23:59 ` Scott Branden 2025-12-12 5:10 ` Tuo Li 0 siblings, 1 reply; 7+ messages in thread From: Scott Branden @ 2025-12-11 23:59 UTC (permalink / raw) To: Tuo Li; +Cc: bcm-kernel-feedback-list, arnd, gregkh, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4340 bytes --] Hi Tuo, On Thu, Dec 11, 2025 at 3:42 PM Tuo Li <islituo@gmail.com> wrote: > > Hi Scott, > > Thanks for your reply! > > On Fri, Dec 12, 2025 at 3:18 AM Scott Branden > <scott.branden@broadcom.com> wrote: > > > > Hi Tuo, > > > > On Wed, Dec 10, 2025 at 10:36 PM Tuo Li <islituo@gmail.com> wrote: > > > > > > In the function bcm_vk_read(), the pointer entry is checked, indicating > > > that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the > > > following code may cause null-pointer dereferences: > > > > > > struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > set_msg_id(&tmp_msg, entry->usr_msg_id); > > > tmp_msg.size = entry->to_h_blks - 1; > > > > > > To prevent these possible null-pointer dereferences, copy to_h_msg, > > > usr_msg_id, and to_h_blks from iter into temporary variables, and return > > > these temporary variables to the application instead of accessing them > > > through a potentially NULL entry. > > > > > > Signed-off-by: Tuo Li <islituo@gmail.com> > > > --- > > > drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > index 1f42d1d5a630..665a3888708a 100644 > > > --- a/drivers/misc/bcm-vk/bcm_vk_msg.c > > > +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > struct device *dev = &vk->pdev->dev; > > > struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; > > > struct bcm_vk_wkent *entry = NULL, *iter; > > > + struct vk_msg_blk tmp_msg; > > > + u32 tmp_usr_msg_id; > > > + u32 tmp_blks; > > > u32 q_num; > > > u32 rsp_length; > > > > > > @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > entry = iter; > > > } else { > > > /* buffer not big enough */ > > > + tmp_msg = iter->to_h_msg[0]; > > > + tmp_usr_msg_id = iter->usr_msg_id; > > > + tmp_blks = iter->to_h_blks; > > > rc = -EMSGSIZE; > > > } > > > goto read_loop_exit; > > > @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > > bcm_vk_free_wkent(dev, entry); > > > } else if (rc == -EMSGSIZE) { > > > - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > - > > > /* > > > * in this case, return just the first block, so > > > * that app knows what size it is looking for. > > > */ > > > - set_msg_id(&tmp_msg, entry->usr_msg_id); > > > - tmp_msg.size = entry->to_h_blks - 1; > > You can just change entry to iter on these 2 lines here? > > > > > + set_msg_id(&tmp_msg, tmp_usr_msg_id); > > > + tmp_msg.size = tmp_blks - 1; > > > if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { > > > dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); > > > rc = -EFAULT; > > > -- > > > 2.43.0 > > > > > Thanks for the suggestion! My understanding is that you are suggesting > replacing entry with iter in the rc == -EMSGSIZE path. However, I am not > sure whether the objects referenced by iter can be modified or freed by > other threads, since this path runs outside the protection of chan->pendq > lock. If that can happen, accessing the objects via iter may lead to a data > race or a use-after-free. > > Could you please help confirm whether this scenario is possible? If my > concern is unfounded, I will prepare a v2 patch and replace entry with > iter as you suggested. > > Thanks again for your time and helpful feedback! > > Sincerely, > Tuo Li Portions of your response sound like it is coming from an AI tool. Are you using such? I am just commenting on your patch. Could you please examine the code to answer your questions? Regards, Scott [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 5473 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() 2025-12-11 23:59 ` Scott Branden @ 2025-12-12 5:10 ` Tuo Li 2025-12-12 18:34 ` Scott Branden 0 siblings, 1 reply; 7+ messages in thread From: Tuo Li @ 2025-12-12 5:10 UTC (permalink / raw) To: Scott Branden; +Cc: bcm-kernel-feedback-list, arnd, gregkh, linux-kernel Hi Scott, On Fri, Dec 12, 2025 at 8:00 AM Scott Branden <scott.branden@broadcom.com> wrote: > > Hi Tuo, > > On Thu, Dec 11, 2025 at 3:42 PM Tuo Li <islituo@gmail.com> wrote: > > > > Hi Scott, > > > > Thanks for your reply! > > > > On Fri, Dec 12, 2025 at 3:18 AM Scott Branden > > <scott.branden@broadcom.com> wrote: > > > > > > Hi Tuo, > > > > > > On Wed, Dec 10, 2025 at 10:36 PM Tuo Li <islituo@gmail.com> wrote: > > > > > > > > In the function bcm_vk_read(), the pointer entry is checked, indicating > > > > that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the > > > > following code may cause null-pointer dereferences: > > > > > > > > struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > > set_msg_id(&tmp_msg, entry->usr_msg_id); > > > > tmp_msg.size = entry->to_h_blks - 1; > > > > > > > > To prevent these possible null-pointer dereferences, copy to_h_msg, > > > > usr_msg_id, and to_h_blks from iter into temporary variables, and return > > > > these temporary variables to the application instead of accessing them > > > > through a potentially NULL entry. > > > > > > > > Signed-off-by: Tuo Li <islituo@gmail.com> > > > > --- > > > > drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > index 1f42d1d5a630..665a3888708a 100644 > > > > --- a/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > struct device *dev = &vk->pdev->dev; > > > > struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; > > > > struct bcm_vk_wkent *entry = NULL, *iter; > > > > + struct vk_msg_blk tmp_msg; > > > > + u32 tmp_usr_msg_id; > > > > + u32 tmp_blks; > > > > u32 q_num; > > > > u32 rsp_length; > > > > > > > > @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > entry = iter; > > > > } else { > > > > /* buffer not big enough */ > > > > + tmp_msg = iter->to_h_msg[0]; > > > > + tmp_usr_msg_id = iter->usr_msg_id; > > > > + tmp_blks = iter->to_h_blks; > > > > rc = -EMSGSIZE; > > > > } > > > > goto read_loop_exit; > > > > @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > > > > bcm_vk_free_wkent(dev, entry); > > > > } else if (rc == -EMSGSIZE) { > > > > - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > > - > > > > /* > > > > * in this case, return just the first block, so > > > > * that app knows what size it is looking for. > > > > */ > > > > - set_msg_id(&tmp_msg, entry->usr_msg_id); > > > > - tmp_msg.size = entry->to_h_blks - 1; > > > You can just change entry to iter on these 2 lines here? > > > > > > > + set_msg_id(&tmp_msg, tmp_usr_msg_id); > > > > + tmp_msg.size = tmp_blks - 1; > > > > if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { > > > > dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); > > > > rc = -EFAULT; > > > > -- > > > > 2.43.0 > > > > > > > > Thanks for the suggestion! My understanding is that you are suggesting > > replacing entry with iter in the rc == -EMSGSIZE path. However, I am not > > sure whether the objects referenced by iter can be modified or freed by > > other threads, since this path runs outside the protection of chan->pendq > > lock. If that can happen, accessing the objects via iter may lead to a data > > race or a use-after-free. > > > > Could you please help confirm whether this scenario is possible? If my > > concern is unfounded, I will prepare a v2 patch and replace entry with > > iter as you suggested. > > > > Thanks again for your time and helpful feedback! > > > > Sincerely, > > Tuo Li > > Portions of your response sound like it is coming from an AI tool. > Are you using such? > I am just commenting on your patch. > Could you please examine the code to answer your questions? > > Regards, > Scott Thanks for pointing it out. I manually analyzed the code, wrote the patch and response, but I did polish them through an AI tool because my English is not great. I am sorry if that made the message look odd. Sincerely, Tuo Li ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() 2025-12-12 5:10 ` Tuo Li @ 2025-12-12 18:34 ` Scott Branden 2025-12-13 2:54 ` Tuo Li 0 siblings, 1 reply; 7+ messages in thread From: Scott Branden @ 2025-12-12 18:34 UTC (permalink / raw) To: Tuo Li; +Cc: bcm-kernel-feedback-list, arnd, gregkh, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5351 bytes --] Hi Tuo, On Thu, Dec 11, 2025 at 9:10 PM Tuo Li <islituo@gmail.com> wrote: > > Hi Scott, > > On Fri, Dec 12, 2025 at 8:00 AM Scott Branden > <scott.branden@broadcom.com> wrote: > > > > Hi Tuo, > > > > On Thu, Dec 11, 2025 at 3:42 PM Tuo Li <islituo@gmail.com> wrote: > > > > > > Hi Scott, > > > > > > Thanks for your reply! > > > > > > On Fri, Dec 12, 2025 at 3:18 AM Scott Branden > > > <scott.branden@broadcom.com> wrote: > > > > > > > > Hi Tuo, > > > > > > > > On Wed, Dec 10, 2025 at 10:36 PM Tuo Li <islituo@gmail.com> wrote: > > > > > > > > > > In the function bcm_vk_read(), the pointer entry is checked, indicating > > > > > that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the > > > > > following code may cause null-pointer dereferences: > > > > > > > > > > struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > > > set_msg_id(&tmp_msg, entry->usr_msg_id); > > > > > tmp_msg.size = entry->to_h_blks - 1; > > > > > > > > > > To prevent these possible null-pointer dereferences, copy to_h_msg, > > > > > usr_msg_id, and to_h_blks from iter into temporary variables, and return > > > > > these temporary variables to the application instead of accessing them > > > > > through a potentially NULL entry. > > > > > > > > > > Signed-off-by: Tuo Li <islituo@gmail.com> > > > > > --- > > > > > drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- > > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > > index 1f42d1d5a630..665a3888708a 100644 > > > > > --- a/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > > +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > > @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > struct device *dev = &vk->pdev->dev; > > > > > struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; > > > > > struct bcm_vk_wkent *entry = NULL, *iter; > > > > > + struct vk_msg_blk tmp_msg; > > > > > + u32 tmp_usr_msg_id; > > > > > + u32 tmp_blks; > > > > > u32 q_num; > > > > > u32 rsp_length; > > > > > > > > > > @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > entry = iter; > > > > > } else { > > > > > /* buffer not big enough */ > > > > > + tmp_msg = iter->to_h_msg[0]; > > > > > + tmp_usr_msg_id = iter->usr_msg_id; > > > > > + tmp_blks = iter->to_h_blks; > > > > > rc = -EMSGSIZE; > > > > > } > > > > > goto read_loop_exit; > > > > > @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > > > > > > bcm_vk_free_wkent(dev, entry); > > > > > } else if (rc == -EMSGSIZE) { > > > > > - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > > > - > > > > > /* > > > > > * in this case, return just the first block, so > > > > > * that app knows what size it is looking for. > > > > > */ > > > > > - set_msg_id(&tmp_msg, entry->usr_msg_id); > > > > > - tmp_msg.size = entry->to_h_blks - 1; > > > > You can just change entry to iter on these 2 lines here? > > > > > > > > > + set_msg_id(&tmp_msg, tmp_usr_msg_id); > > > > > + tmp_msg.size = tmp_blks - 1; > > > > > if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { > > > > > dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); > > > > > rc = -EFAULT; > > > > > -- > > > > > 2.43.0 > > > > > > > > > > > Thanks for the suggestion! My understanding is that you are suggesting > > > replacing entry with iter in the rc == -EMSGSIZE path. However, I am not > > > sure whether the objects referenced by iter can be modified or freed by > > > other threads, since this path runs outside the protection of chan->pendq > > > lock. If that can happen, accessing the objects via iter may lead to a data > > > race or a use-after-free. > > > > > > Could you please help confirm whether this scenario is possible? If my > > > concern is unfounded, I will prepare a v2 patch and replace entry with > > > iter as you suggested. > > > > > > Thanks again for your time and helpful feedback! > > > > > > Sincerely, > > > Tuo Li > > > > Portions of your response sound like it is coming from an AI tool. > > Are you using such? > > I am just commenting on your patch. > > Could you please examine the code to answer your questions? > > > > Regards, > > Scott > > Thanks for pointing it out. > I manually analyzed the code, wrote the patch and response, but I did > polish them through an AI tool because my English is not great. > I am sorry if that made the message look odd. I can see why you went with the approach you did now. Sure, we can go with this patch version. Reviewed-by: Scott Branden <scott.branden@broadcom.com> > > Sincerely, > Tuo Li [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 5473 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() 2025-12-12 18:34 ` Scott Branden @ 2025-12-13 2:54 ` Tuo Li 0 siblings, 0 replies; 7+ messages in thread From: Tuo Li @ 2025-12-13 2:54 UTC (permalink / raw) To: Scott Branden; +Cc: bcm-kernel-feedback-list, arnd, gregkh, linux-kernel Hi Scott, On Sat, Dec 13, 2025 at 2:34 AM Scott Branden <scott.branden@broadcom.com> wrote: > > Hi Tuo, > > On Thu, Dec 11, 2025 at 9:10 PM Tuo Li <islituo@gmail.com> wrote: > > > > Hi Scott, > > > > On Fri, Dec 12, 2025 at 8:00 AM Scott Branden > > <scott.branden@broadcom.com> wrote: > > > > > > Hi Tuo, > > > > > > On Thu, Dec 11, 2025 at 3:42 PM Tuo Li <islituo@gmail.com> wrote: > > > > > > > > Hi Scott, > > > > > > > > Thanks for your reply! > > > > > > > > On Fri, Dec 12, 2025 at 3:18 AM Scott Branden > > > > <scott.branden@broadcom.com> wrote: > > > > > > > > > > Hi Tuo, > > > > > > > > > > On Wed, Dec 10, 2025 at 10:36 PM Tuo Li <islituo@gmail.com> wrote: > > > > > > > > > > > > In the function bcm_vk_read(), the pointer entry is checked, indicating > > > > > > that it can be NULL. If entry is NULL and rc is set to -EMSGSIZE, the > > > > > > following code may cause null-pointer dereferences: > > > > > > > > > > > > struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > > > > set_msg_id(&tmp_msg, entry->usr_msg_id); > > > > > > tmp_msg.size = entry->to_h_blks - 1; > > > > > > > > > > > > To prevent these possible null-pointer dereferences, copy to_h_msg, > > > > > > usr_msg_id, and to_h_blks from iter into temporary variables, and return > > > > > > these temporary variables to the application instead of accessing them > > > > > > through a potentially NULL entry. > > > > > > > > > > > > Signed-off-by: Tuo Li <islituo@gmail.com> > > > > > > --- > > > > > > drivers/misc/bcm-vk/bcm_vk_msg.c | 12 ++++++++---- > > > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/misc/bcm-vk/bcm_vk_msg.c b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > > > index 1f42d1d5a630..665a3888708a 100644 > > > > > > --- a/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > > > +++ b/drivers/misc/bcm-vk/bcm_vk_msg.c > > > > > > @@ -1010,6 +1010,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > > struct device *dev = &vk->pdev->dev; > > > > > > struct bcm_vk_msg_chan *chan = &vk->to_h_msg_chan; > > > > > > struct bcm_vk_wkent *entry = NULL, *iter; > > > > > > + struct vk_msg_blk tmp_msg; > > > > > > + u32 tmp_usr_msg_id; > > > > > > + u32 tmp_blks; > > > > > > u32 q_num; > > > > > > u32 rsp_length; > > > > > > > > > > > > @@ -1034,6 +1037,9 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > > entry = iter; > > > > > > } else { > > > > > > /* buffer not big enough */ > > > > > > + tmp_msg = iter->to_h_msg[0]; > > > > > > + tmp_usr_msg_id = iter->usr_msg_id; > > > > > > + tmp_blks = iter->to_h_blks; > > > > > > rc = -EMSGSIZE; > > > > > > } > > > > > > goto read_loop_exit; > > > > > > @@ -1052,14 +1058,12 @@ ssize_t bcm_vk_read(struct file *p_file, > > > > > > > > > > > > bcm_vk_free_wkent(dev, entry); > > > > > > } else if (rc == -EMSGSIZE) { > > > > > > - struct vk_msg_blk tmp_msg = entry->to_h_msg[0]; > > > > > > - > > > > > > /* > > > > > > * in this case, return just the first block, so > > > > > > * that app knows what size it is looking for. > > > > > > */ > > > > > > - set_msg_id(&tmp_msg, entry->usr_msg_id); > > > > > > - tmp_msg.size = entry->to_h_blks - 1; > > > > > You can just change entry to iter on these 2 lines here? > > > > > > > > > > > + set_msg_id(&tmp_msg, tmp_usr_msg_id); > > > > > > + tmp_msg.size = tmp_blks - 1; > > > > > > if (copy_to_user(buf, &tmp_msg, VK_MSGQ_BLK_SIZE) != 0) { > > > > > > dev_err(dev, "Error return 1st block in -EMSGSIZE\n"); > > > > > > rc = -EFAULT; > > > > > > -- > > > > > > 2.43.0 > > > > > > > > > > > > > > Thanks for the suggestion! My understanding is that you are suggesting > > > > replacing entry with iter in the rc == -EMSGSIZE path. However, I am not > > > > sure whether the objects referenced by iter can be modified or freed by > > > > other threads, since this path runs outside the protection of chan->pendq > > > > lock. If that can happen, accessing the objects via iter may lead to a data > > > > race or a use-after-free. > > > > > > > > Could you please help confirm whether this scenario is possible? If my > > > > concern is unfounded, I will prepare a v2 patch and replace entry with > > > > iter as you suggested. > > > > > > > > Thanks again for your time and helpful feedback! > > > > > > > > Sincerely, > > > > Tuo Li > > > > > > Portions of your response sound like it is coming from an AI tool. > > > Are you using such? > > > I am just commenting on your patch. > > > Could you please examine the code to answer your questions? > > > > > > Regards, > > > Scott > > > > Thanks for pointing it out. > > I manually analyzed the code, wrote the patch and response, but I did > > polish them through an AI tool because my English is not great. > > I am sorry if that made the message look odd. > > I can see why you went with the approach you did now. > Sure, we can go with this patch version. > > Reviewed-by: Scott Branden <scott.branden@broadcom.com> > > > > Sincerely, > > Tuo Li Thank you for your patience and help! Sincerely, Tuo Li ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-12-13 2:54 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-12-11 6:36 [PATCH] misc: bcm_vk: Fix possible null-pointer dereferences in bcm_vk_read() Tuo Li 2025-12-11 19:18 ` Scott Branden 2025-12-11 23:42 ` Tuo Li 2025-12-11 23:59 ` Scott Branden 2025-12-12 5:10 ` Tuo Li 2025-12-12 18:34 ` Scott Branden 2025-12-13 2:54 ` Tuo Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).