Thread: Re: Security issue in linux/drivers/firewire/nosy.c
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2024-04-02 14:33:09
|
Hi Thanassis, Thanks for your contacting to me about the issue. On Tue, Apr 2, 2024, at 22:56, Thanassis Avgerinos wrote: > Dear Sakamoto-san, > > I apologize for the email - I am sure you are very busy. Thank you for > all the work to help maintain firewire in the Linux trunk. > > I will be brief to not waste your time. I am contacting you to report a > possible security issue in firewire/nosy.c:L151 which reads: > > ``` > /* FIXME: Check length <= user_length. */ > > end = buffer->data + buffer->capacity; > length = buffer->head->length; > > if (&buffer->head->data[length] < end) { > if (copy_to_user(data, buffer->head->data, length)) > return -EFAULT; > ``` > > Note that the user-supplied length is not respected at all when reading > bytes from the wire which can lead to a number of issues (including > security ones such as overflows). > > I believe this is potentially serious but also quite fixable. Would you > accept a patch? Should I contact the security team instead? > > Thank you for your time. I'm pleased to review your fix for the issue;) I don't mind that you post it just to me, since any mailing list demands you to subscribe it in advance when posting, and it is a bit cumbersome just to send patches. Additionally, I don't mind the patch format as long as it includes enough fractions to fix the issue. In the case, I'll revise it for the complete patches, alternatively. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-18 08:45:03
|
Hi, On Wed, Apr 17, 2024 at 11:46:47AM -0400, Thanassis Avgerinos wrote: > Hi Takashi, > > Apologies for the delay, and thank you for the super fast response! Please > find attached a possible patch for this issue. It should be > self-explanatory but you may have a better suggestion on how to handle it. > Please let me know and I'm happy to make changes as needed. > > After this is patched, would it be possible to issue a CVE for it? Would > you be the best person to help me with this? > > Best, > Thanassis Thanks for the patch to fix the TODO, however would I ask you to send it again with your `Sign-off-by:` tag? It is mandatory in usual kernel development process, and I can not apply any modification about it, with the respect to copyright, sorry. I have a nitpick about the return value at the case that the user process provides the insufficient size of buffer against the data stored in kernel space. In the case, the typical UNIX-like kernel returns zero instead of -EINVAL. We should follow to the convention if we have no specific reason, in my opinion. > From f7ee97fabe1519225ba30fd9454344d0a75f4d94 Mon Sep 17 00:00:00 2001 > From: Thanassis Avgerinos <tha...@gm...> > Date: Wed, 17 Apr 2024 11:30:02 -0400 > Subject: [PATCH] firewire: nosy: ensure user_length is taken into account when > fetching packet contents > > Ensure that packet_buffer_get respects the user_length provided. If > the length of the head packet exceeds the user_length, packet_buffer_get > will now return -EINVAL to signify to the user that a larger data > buffer is required. Helps prevent user space overflows. > --- > drivers/firewire/nosy.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/firewire/nosy.c b/drivers/firewire/nosy.c > index b0d671db178a..b1dccf3ba8f9 100644 > --- a/drivers/firewire/nosy.c > +++ b/drivers/firewire/nosy.c > @@ -148,10 +148,12 @@ packet_buffer_get(struct client *client, char __user *data, size_t user_length) > if (atomic_read(&buffer->size) == 0) > return -ENODEV; > > - /* FIXME: Check length <= user_length. */ > + length = buffer->head->length; > + > + if (length > user_length) > + return -EINVAL; > > end = buffer->data + buffer->capacity; > - length = buffer->head->length; > > if (&buffer->head->data[length] < end) { > if (copy_to_user(data, buffer->head->data, length)) > -- > 2.23.0 Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-04-29 09:42:36
|
Hi, On Thu, Apr 18, 2024 at 11:18:08AM -0400, Thanassis Avgerinos wrote: > Resending as I forgot to reply-all. > > Best, > Thanassis I'm sorry for my late reply. I applied the patch to for-linus branch. I'll send it as the part of fixes for v6.9-rc7 in this weekend. Thanks Takashi Sakamoto |
From: Takashi S. <o-t...@sa...> - 2024-05-02 12:13:42
|
Hi, On Wed, May 01, 2024 at 11:17:44AM -0400, Thanassis Avgerinos wrote: > Perfect, thank you for the update and getting the fix in Takashi! > > Who would be the right person to contact to get a CVE number for this issue > and track remediation downstream? I'm sorry but I scarcely know the CVE process in kernel development. I think that the entry point is to refer to documentation about it[1], recently added to kernel tree. Jonathan Corbet wrote an article about the kernel CNA[2]. It would be a good resource to you. [1] https://docs.kernel.org/process/cve.html [2] https://lwn.net/Articles/961978/ Regards Takashi Sakamoto |