* [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
@ 2022-06-02 18:32 gerbert
2022-06-04 17:44 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: gerbert @ 2022-06-02 18:32 UTC (permalink / raw)
To: u-boot
This patch tries to fix a CVE-2019-14196 fix
In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
to transfer a reply data of NFS_FHSIZE size. Since the data field in
struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
the size of data array won't change in the future).
What concerns if-condition for NFSV3_FLAG, since filefh3_length is
signed integer, it may carry negative values which may lead to memcpy
failure, so in this case we need to introduce not only boundary check
(filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
filefh3_length is not negative.
Signed-off-by: gerbert <gerbert@users.noreply.github.com>
---
net/nfs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c
index 9152ab742e..5186130ea9 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned
len)
}
if (supported_nfs_versions & NFSV2_FLAG) {
- if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt)
+ NFS_FHSIZE) > len)
- return -NFS_RPC_DROP;
memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
} else { /* NFSV3_FLAG */
filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
+ if (filefh3_length < 0)
+ return -NFS_RPC_DROP;
if (filefh3_length > NFS3_FHSIZE)
- filefh3_length = NFS3_FHSIZE;
+ filefh3_length = NFS3_FHSIZE;
memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
}
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
2022-06-02 18:32 [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check gerbert
@ 2022-06-04 17:44 ` Heinrich Schuchardt
2022-06-04 18:07 ` gerbert
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2022-06-04 17:44 UTC (permalink / raw)
To: gerbert; +Cc: u-boot
On 6/2/22 20:32, gerbert wrote:
> This patch tries to fix a CVE-2019-14196 fix
>
> In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
> to transfer a reply data of NFS_FHSIZE size. Since the data field in
> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
> the size of data array won't change in the future).
>
> What concerns if-condition for NFSV3_FLAG, since filefh3_length is
> signed integer, it may carry negative values which may lead to memcpy
> failure, so in this case we need to introduce not only boundary check
> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
> filefh3_length is not negative.
>
> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
> ---
> net/nfs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/nfs.c b/net/nfs.c
> index 9152ab742e..5186130ea9 100644
> --- a/net/nfs.c
> +++ b/net/nfs.c
> @@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned len)
> }
>
> if (supported_nfs_versions & NFSV2_FLAG) {
> - if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt)
> + NFS_FHSIZE) > len)
> - return -NFS_RPC_DROP;
> memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
> } else { /* NFSV3_FLAG */
> filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
> + if (filefh3_length < 0)
This is the definition:
static unsigned int filefh3_length
The value cannot be negative.
Cf.
bdbf7a05e26f3c5 ("net: nfs: Fix CVE-2022-30767 (old CVE-2019-14196)")
> + return -NFS_RPC_DROP;
> if (filefh3_length > NFS3_FHSIZE)
> - filefh3_length = NFS3_FHSIZE;
> + filefh3_length = NFS3_FHSIZE;
This seems to be an unrelated change.
Best regards
Heinrich
> memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
2022-06-04 17:44 ` Heinrich Schuchardt
@ 2022-06-04 18:07 ` gerbert
0 siblings, 0 replies; 6+ messages in thread
From: gerbert @ 2022-06-04 18:07 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot
Heinrich Schuchardt писал 2022-06-04 20:44:
> On 6/2/22 20:32, gerbert wrote:
>> This patch tries to fix a CVE-2019-14196 fix
>>
>> In if-condition, where NFSV2_FLAG is checked, memcpy call is
>> performed
>> to transfer a reply data of NFS_FHSIZE size. Since the data field in
>> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
>> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write
>> (considering
>> the size of data array won't change in the future).
>>
>> What concerns if-condition for NFSV3_FLAG, since filefh3_length is
>> signed integer, it may carry negative values which may lead to memcpy
>> failure, so in this case we need to introduce not only boundary check
>> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
>> filefh3_length is not negative.
>>
>> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
>> ---
>> net/nfs.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/nfs.c b/net/nfs.c
>> index 9152ab742e..5186130ea9 100644
>> --- a/net/nfs.c
>> +++ b/net/nfs.c
>> @@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned
>> len)
>> }
>>
>> if (supported_nfs_versions & NFSV2_FLAG) {
>> - if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar
>> *)(&rpc_pkt)
>> + NFS_FHSIZE) > len)
>> - return -NFS_RPC_DROP;
>> memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
>> } else { /* NFSV3_FLAG */
>> filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
>> + if (filefh3_length < 0)
>
> This is the definition:
>
> static unsigned int filefh3_length
> The value cannot be negative.
That's right. It's unsigned now. Unfortunately I was looking into the
version
I was using in the project, which is, 2021.01. A bit outdated and has
this field
as signed integer. That's why I was thinking that this should work.
Meanwhile, I then think that this part can be removed:
>> - if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar
>> *)(&rpc_pkt)
>> + NFS_FHSIZE) > len)
as
>> memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
won't leave the boundary of 'rpc_pkt.u.reply.data + 1' due to the size
of NFS_FHSIZE.
>
> Cf.
> bdbf7a05e26f3c5 ("net: nfs: Fix CVE-2022-30767 (old CVE-2019-14196)")
>
>
>> + return -NFS_RPC_DROP;
>> if (filefh3_length > NFS3_FHSIZE)
>> - filefh3_length = NFS3_FHSIZE;
>> + filefh3_length = NFS3_FHSIZE;
>
> This seems to be an unrelated change.
Thought about a little cosmetics here.
>
> Best regards
>
> Heinrich
>
>> memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
>> }
>>
Kind regards,
Gerbert
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
2022-06-06 14:43 ` Tom Rini
@ 2022-06-06 15:10 ` gerbert
0 siblings, 0 replies; 6+ messages in thread
From: gerbert @ 2022-06-06 15:10 UTC (permalink / raw)
To: Tom Rini; +Cc: u-boot
Tom Rini писал 2022-06-06 17:43:
> On Thu, Jun 02, 2022 at 09:18:42PM +0300, gerbert wrote:
>
>> This patch tries to fix a CVE-2019-14196 fix
>>
>> In if-condition, where NFSV2_FLAG is checked, memcpy call is
>> performed
>> to transfer a reply data of NFS_FHSIZE size. Since the data field in
>> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
>> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write
>> (considering
>> the size of data array won't change in the future).
>>
>> What concerns if-condition for NFSV3_FLAG, since filefh3_length is
>> signed integer, it may carry negative values which may lead to memcpy
>> failure, so in this case we need to introduce not only boundary check
>> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
>> filefh3_length is not negative.
>>
>> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
>
> This has been addressed as:
> https://patchwork.ozlabs.org/project/uboot/patch/20220518163103.372-1-zi0Black@protonmail.com/
> and more clearly:
> https://source.denx.de/u-boot/u-boot/-/commit/bdbf7a05e26f3c5fd437c99e2755ffde186ddc80
> recently, thanks.
That's right. But as far as I can see if-condition for NFS v2 has the
same leftover,
which was removed from v3 part. In this case I guess it can be removed.
Kind regards,
Gerbert
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
2022-06-02 18:18 gerbert
@ 2022-06-06 14:43 ` Tom Rini
2022-06-06 15:10 ` gerbert
0 siblings, 1 reply; 6+ messages in thread
From: Tom Rini @ 2022-06-06 14:43 UTC (permalink / raw)
To: gerbert; +Cc: u-boot
[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]
On Thu, Jun 02, 2022 at 09:18:42PM +0300, gerbert wrote:
> This patch tries to fix a CVE-2019-14196 fix
>
> In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
> to transfer a reply data of NFS_FHSIZE size. Since the data field in
> struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
> NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
> the size of data array won't change in the future).
>
> What concerns if-condition for NFSV3_FLAG, since filefh3_length is
> signed integer, it may carry negative values which may lead to memcpy
> failure, so in this case we need to introduce not only boundary check
> (filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
> filefh3_length is not negative.
>
> Signed-off-by: gerbert <gerbert@users.noreply.github.com>
This has been addressed as:
https://patchwork.ozlabs.org/project/uboot/patch/20220518163103.372-1-zi0Black@protonmail.com/
and more clearly:
https://source.denx.de/u-boot/u-boot/-/commit/bdbf7a05e26f3c5fd437c99e2755ffde186ddc80
recently, thanks.
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check
@ 2022-06-02 18:18 gerbert
2022-06-06 14:43 ` Tom Rini
0 siblings, 1 reply; 6+ messages in thread
From: gerbert @ 2022-06-02 18:18 UTC (permalink / raw)
To: u-boot
This patch tries to fix a CVE-2019-14196 fix
In if-condition, where NFSV2_FLAG is checked, memcpy call is performed
to transfer a reply data of NFS_FHSIZE size. Since the data field in
struct rpc_t structure has the size of (1024 / 4) + 26 = 282, while
NFS_FHSIZE is only 32, it won't lead to out-of-bounds write (considering
the size of data array won't change in the future).
What concerns if-condition for NFSV3_FLAG, since filefh3_length is
signed integer, it may carry negative values which may lead to memcpy
failure, so in this case we need to introduce not only boundary check
(filefh3_length > NFS3_FHSIZE), which exists, but also make sure that
filefh3_length is not negative.
Signed-off-by: gerbert <gerbert@users.noreply.github.com>
---
net/nfs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/nfs.c b/net/nfs.c
index 9152ab742e..5186130ea9 100644
--- a/net/nfs.c
+++ b/net/nfs.c
@@ -566,13 +566,13 @@ static int nfs_lookup_reply(uchar *pkt, unsigned
len)
}
if (supported_nfs_versions & NFSV2_FLAG) {
- if (((uchar *)&(rpc_pkt.u.reply.data[0]) - (uchar *)(&rpc_pkt) +
NFS_FHSIZE) > len)
- return -NFS_RPC_DROP;
memcpy(filefh, rpc_pkt.u.reply.data + 1, NFS_FHSIZE);
} else { /* NFSV3_FLAG */
filefh3_length = ntohl(rpc_pkt.u.reply.data[1]);
+ if (filefh3_length < 0)
+ return -NFS_RPC_DROP;
if (filefh3_length > NFS3_FHSIZE)
- filefh3_length = NFS3_FHSIZE;
+ filefh3_length = NFS3_FHSIZE;
memcpy(filefh, rpc_pkt.u.reply.data + 2, filefh3_length);
}
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-06 15:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 18:32 [PATCH 1/1] CVE-2022-30767: unbounded memcpy with a failed length check gerbert
2022-06-04 17:44 ` Heinrich Schuchardt
2022-06-04 18:07 ` gerbert
-- strict thread matches above, loose matches on Subject: below --
2022-06-02 18:18 gerbert
2022-06-06 14:43 ` Tom Rini
2022-06-06 15:10 ` gerbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.