Use checked_add() and checked_mul() when computing offsets from
firmware-provided values in new_fwsec().
Without checked arithmetic, corrupt firmware could cause integer overflow. The
danger is not just wrapping to a huge value, but potentially wrapping to a
small plausible offset that passes validation yet accesses entirely wrong data,
causing silent corruption or security issues.
Reviewed-by: Zhi Wang <zhiw@nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
drivers/gpu/nova-core/firmware/fwsec.rs | 60 ++++++++++++++-----------
1 file changed, 35 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
index a8ec08a500ac..71541b1f07d7 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -46,10 +46,7 @@
Signed,
Unsigned, //
},
- num::{
- FromSafeCast,
- IntoSafeCast, //
- },
+ num::FromSafeCast,
vbios::Vbios,
};
@@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
let ucode = bios.fwsec_image().ucode(&desc)?;
let mut dma_object = DmaObject::from_data(dev, ucode)?;
- let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
+ // Compute hdr_offset = imem_load_size + interface_offset.
+ let hdr_offset = desc
+ .imem_load_size()
+ .checked_add(desc.interface_offset())
+ .map(usize::from_safe_cast)
+ .ok_or(EINVAL)?;
// SAFETY: we have exclusive access to `dma_object`.
let hdr: &FalconAppifHdrV1 = unsafe { transmute(&dma_object, hdr_offset) }?;
@@ -277,26 +279,28 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
// Find the DMEM mapper section in the firmware.
for i in 0..usize::from(hdr.entry_count) {
+ // Compute entry_offset = hdr_offset + header_size + i * entry_size.
+ let entry_offset = hdr_offset
+ .checked_add(usize::from(hdr.header_size))
+ .and_then(|o| o.checked_add(i.checked_mul(usize::from(hdr.entry_size))?))
+ .ok_or(EINVAL)?;
// SAFETY: we have exclusive access to `dma_object`.
- let app: &FalconAppifV1 = unsafe {
- transmute(
- &dma_object,
- hdr_offset + usize::from(hdr.header_size) + i * usize::from(hdr.entry_size),
- )
- }?;
+ let app: &FalconAppifV1 = unsafe { transmute(&dma_object, entry_offset) }?;
if app.id != NVFW_FALCON_APPIF_ID_DMEMMAPPER {
continue;
}
let dmem_base = app.dmem_base;
+ // Compute dmem_mapper_offset = imem_load_size + dmem_base.
+ let dmem_mapper_offset = desc
+ .imem_load_size()
+ .checked_add(dmem_base)
+ .map(usize::from_safe_cast)
+ .ok_or(EINVAL)?;
// SAFETY: we have exclusive access to `dma_object`.
- let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
- transmute_mut(
- &mut dma_object,
- (desc.imem_load_size() + dmem_base).into_safe_cast(),
- )
- }?;
+ let dmem_mapper: &mut FalconAppifDmemmapperV3 =
+ unsafe { transmute_mut(&mut dma_object, dmem_mapper_offset) }?;
dmem_mapper.init_cmd = match cmd {
FwsecCommand::Frts { .. } => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS,
@@ -304,13 +308,15 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
};
let cmd_in_buffer_offset = dmem_mapper.cmd_in_buffer_offset;
+ // Compute frts_cmd_offset = imem_load_size + cmd_in_buffer_offset.
+ let frts_cmd_offset = desc
+ .imem_load_size()
+ .checked_add(cmd_in_buffer_offset)
+ .map(usize::from_safe_cast)
+ .ok_or(EINVAL)?;
// SAFETY: we have exclusive access to `dma_object`.
- let frts_cmd: &mut FrtsCmd = unsafe {
- transmute_mut(
- &mut dma_object,
- (desc.imem_load_size() + cmd_in_buffer_offset).into_safe_cast(),
- )
- }?;
+ let frts_cmd: &mut FrtsCmd =
+ unsafe { transmute_mut(&mut dma_object, frts_cmd_offset) }?;
frts_cmd.read_vbios = ReadVbios {
ver: 1,
@@ -356,8 +362,12 @@ pub(crate) fn new(
// Patch signature if needed.
let desc = bios.fwsec_image().header()?;
let ucode_signed = if desc.signature_count() != 0 {
- let sig_base_img =
- usize::from_safe_cast(desc.imem_load_size() + desc.pkc_data_offset());
+ // Compute sig_base_img = imem_load_size + pkc_data_offset.
+ let sig_base_img = desc
+ .imem_load_size()
+ .checked_add(desc.pkc_data_offset())
+ .map(usize::from_safe_cast)
+ .ok_or(EINVAL)?;
let desc_sig_versions = u32::from(desc.signature_versions());
let reg_fuse_version =
falcon.signature_reg_fuse_version(bar, desc.engine_id_mask(), desc.ucode_id())?;
--
2.34.1On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote:
> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
> let ucode = bios.fwsec_image().ucode(&desc)?;
> let mut dma_object = DmaObject::from_data(dev, ucode)?;
>
> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
> + // Compute hdr_offset = imem_load_size + interface_offset.
I do get the idea behind those comments, but are we sure that's really a good
idea? How do we ensure to keep them up to date in case we have to change the
code?
If we really want this, I'd at least chose a common syntax, e.g.
// CALC: `imem_load_size + interface_offset`
without the variable name the resulting value is assigned to.
But I'd rather prefer to just drop those comments.
> + let hdr_offset = desc
> + .imem_load_size()
> + .checked_add(desc.interface_offset())
> + .map(usize::from_safe_cast)
> + .ok_or(EINVAL)?;
> // SAFETY: we have exclusive access to `dma_object`.
> let hdr: &FalconAppifHdrV1 = unsafe { transmute(&dma_object, hdr_offset) }?;
On 1/28/2026 5:53 AM, Danilo Krummrich wrote: > On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote: >> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re >> let ucode = bios.fwsec_image().ucode(&desc)?; >> let mut dma_object = DmaObject::from_data(dev, ucode)?; >> >> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset()); >> + // Compute hdr_offset = imem_load_size + interface_offset. > > I do get the idea behind those comments, but are we sure that's really a good > idea? How do we ensure to keep them up to date in case we have to change the > code? > > If we really want this, I'd at least chose a common syntax, e.g. > > // CALC: `imem_load_size + interface_offset` > > without the variable name the resulting value is assigned to. > > But I'd rather prefer to just drop those comments. The idea of adding these comments was to improve readability. However, I can drop them in the v3, that's fine with me. Do you want me to wait for additional comments on this series, or should I make the update and repost it? Thanks, -- Joel Fernandes
On Wed Jan 28, 2026 at 4:14 PM CET, Joel Fernandes wrote: > On 1/28/2026 5:53 AM, Danilo Krummrich wrote: >> On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote: >>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re >>> let ucode = bios.fwsec_image().ucode(&desc)?; >>> let mut dma_object = DmaObject::from_data(dev, ucode)?; >>> >>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset()); >>> + // Compute hdr_offset = imem_load_size + interface_offset. >> >> I do get the idea behind those comments, but are we sure that's really a good >> idea? How do we ensure to keep them up to date in case we have to change the >> code? >> >> If we really want this, I'd at least chose a common syntax, e.g. >> >> // CALC: `imem_load_size + interface_offset` >> >> without the variable name the resulting value is assigned to. >> >> But I'd rather prefer to just drop those comments. > The idea of adding these comments was to improve readability. However, I > can drop them in the v3, that's fine with me. Yeah, that's why I wrote "I get the idea". :) But as I write above, I'm concerned about the comments getting outdated or inconsistent over time. Besides that, it more seems like something your favorite editor should help with instead. > Do you want me to wait for additional comments on this series, or should > I make the update and repost it? Thanks, As mentioned, I tend to think we should just drop them, but I'm happy to hear some more opinions on this if any.
On 1/28/26 4:20 PM, Danilo Krummrich wrote: > On Wed Jan 28, 2026 at 4:14 PM CET, Joel Fernandes wrote: >> On 1/28/2026 5:53 AM, Danilo Krummrich wrote: >>> On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote: >>>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re >>>> let ucode = bios.fwsec_image().ucode(&desc)?; >>>> let mut dma_object = DmaObject::from_data(dev, ucode)?; >>>> >>>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset()); >>>> + // Compute hdr_offset = imem_load_size + interface_offset. >>> >>> I do get the idea behind those comments, but are we sure that's really a good >>> idea? How do we ensure to keep them up to date in case we have to change the >>> code? >>> >>> If we really want this, I'd at least chose a common syntax, e.g. >>> >>> // CALC: `imem_load_size + interface_offset` >>> >>> without the variable name the resulting value is assigned to. >>> >>> But I'd rather prefer to just drop those comments. >> The idea of adding these comments was to improve readability. However, I >> can drop them in the v3, that's fine with me. > > Yeah, that's why I wrote "I get the idea". :) But as I write above, I'm > concerned about the comments getting outdated or inconsistent over time. > > Besides that, it more seems like something your favorite editor should help with > instead. > >> Do you want me to wait for additional comments on this series, or should >> I make the update and repost it? Thanks, > > As mentioned, I tend to think we should just drop them, but I'm happy to hear > some more opinions on this if any. Yes, please drop the comments. They were just echoing the code for the most part, so the code itself will be easier to read without them. I think this is something that, when you are writing the code and comments, it's really hard to see. But when you come back a while later (or you are reviewing someone else's patch), then it is much easier to spot. thanks, -- John Hubbard
On Thu Jan 29, 2026 at 9:58 AM JST, John Hubbard wrote:
> On 1/28/26 4:20 PM, Danilo Krummrich wrote:
>> On Wed Jan 28, 2026 at 4:14 PM CET, Joel Fernandes wrote:
>>> On 1/28/2026 5:53 AM, Danilo Krummrich wrote:
>>>> On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote:
>>>>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>>>> let ucode = bios.fwsec_image().ucode(&desc)?;
>>>>> let mut dma_object = DmaObject::from_data(dev, ucode)?;
>>>>>
>>>>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
>>>>> + // Compute hdr_offset = imem_load_size + interface_offset.
>>>>
>>>> I do get the idea behind those comments, but are we sure that's really a good
>>>> idea? How do we ensure to keep them up to date in case we have to change the
>>>> code?
>>>>
>>>> If we really want this, I'd at least chose a common syntax, e.g.
>>>>
>>>> // CALC: `imem_load_size + interface_offset`
>>>>
>>>> without the variable name the resulting value is assigned to.
>>>>
>>>> But I'd rather prefer to just drop those comments.
>>> The idea of adding these comments was to improve readability. However, I
>>> can drop them in the v3, that's fine with me.
>>
>> Yeah, that's why I wrote "I get the idea". :) But as I write above, I'm
>> concerned about the comments getting outdated or inconsistent over time.
>>
>> Besides that, it more seems like something your favorite editor should help with
>> instead.
>>
>>> Do you want me to wait for additional comments on this series, or should
>>> I make the update and repost it? Thanks,
>>
>> As mentioned, I tend to think we should just drop them, but I'm happy to hear
>> some more opinions on this if any.
>
> Yes, please drop the comments. They were just echoing the code for
> the most part, so the code itself will be easier to read without
> them.
I agree that if the operation is a simple `checked_add`, then comments
are not necessarily useful.
However, we also have stuff like
let entry_offset = hdr_offset
.checked_add(usize::from(hdr.header_size))
.and_then(|o| o.checked_add(i.checked_mul(usize::from(hdr.entry_size))?))
Where the order of operation matters, and for these I think it is safer
to have a quick confirmation.
Thus for anything non-trivial, I'd like to keep a `// CALC: ` header
describing the intended operation. I also noticed that LLMs check that
the code is in accordance with comments, which provides an additional
layer of checking.
On Tue, Feb 3, 2026 at 11:25 PM Alexandre Courbot <acourbot@nvidia.com> wrote: > > Thus for anything non-trivial, I'd like to keep a `// CALC: ` header > describing the intended operation. I also noticed that LLMs check that > the code is in accordance with comments, which provides an additional > layer of checking. Yeah, it is the same reason why documentation as well as other tagged comments like `// SAFETY:` comments enable to catch mistakes even if they may be redundant in a certain sense. I wouldn't mind having those tagged comments after a certain complexity -- perhaps it could be possible to define a heuristic for a threshold where such a comment is required, and get Clippy to warn about it (we are trying to get other tagged comments implemented, so it is a good opportunity). I guess a fancy IDE could perhaps render the math expression on hover as well. Cheers, Miguel
On Thu Jan 29, 2026 at 9:20 AM JST, Danilo Krummrich wrote: > On Wed Jan 28, 2026 at 4:14 PM CET, Joel Fernandes wrote: >> On 1/28/2026 5:53 AM, Danilo Krummrich wrote: >>> On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote: >>>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re >>>> let ucode = bios.fwsec_image().ucode(&desc)?; >>>> let mut dma_object = DmaObject::from_data(dev, ucode)?; >>>> >>>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset()); >>>> + // Compute hdr_offset = imem_load_size + interface_offset. >>> >>> I do get the idea behind those comments, but are we sure that's really a good >>> idea? How do we ensure to keep them up to date in case we have to change the >>> code? >>> >>> If we really want this, I'd at least chose a common syntax, e.g. >>> >>> // CALC: `imem_load_size + interface_offset` >>> >>> without the variable name the resulting value is assigned to. >>> >>> But I'd rather prefer to just drop those comments. >> The idea of adding these comments was to improve readability. However, I >> can drop them in the v3, that's fine with me. > > Yeah, that's why I wrote "I get the idea". :) But as I write above, I'm > concerned about the comments getting outdated or inconsistent over time. > > Besides that, it more seems like something your favorite editor should help with > instead. > >> Do you want me to wait for additional comments on this series, or should >> I make the update and repost it? Thanks, > > As mentioned, I tend to think we should just drop them, but I'm happy to hear > some more opinions on this if any. For safety I would keep something like the // CALC: `imem_load_size + interface_offset` you suggested. From simple operations yes, the code would be obvious, but there are also more involved computations where order matters and it is good to have a reference. These shouldn't change often anyway, and the `CALC:` header catches the attention of anyone who would update them, similarly to a `SAFETY:` comment. If Joel agrees, I will amend the comments accordingly in my staging branch.
On 1/28/2026 7:36 PM, Alexandre Courbot wrote: > On Thu Jan 29, 2026 at 9:20 AM JST, Danilo Krummrich wrote: >> On Wed Jan 28, 2026 at 4:14 PM CET, Joel Fernandes wrote: >>> On 1/28/2026 5:53 AM, Danilo Krummrich wrote: >>>> On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote: >>>>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re >>>>> let ucode = bios.fwsec_image().ucode(&desc)?; >>>>> let mut dma_object = DmaObject::from_data(dev, ucode)?; >>>>> >>>>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset()); >>>>> + // Compute hdr_offset = imem_load_size + interface_offset. >>>> >>>> I do get the idea behind those comments, but are we sure that's really a good >>>> idea? How do we ensure to keep them up to date in case we have to change the >>>> code? >>>> >>>> If we really want this, I'd at least chose a common syntax, e.g. >>>> >>>> // CALC: `imem_load_size + interface_offset` >>>> >>>> without the variable name the resulting value is assigned to. >>>> >>>> But I'd rather prefer to just drop those comments. >>> The idea of adding these comments was to improve readability. However, I >>> can drop them in the v3, that's fine with me. >> >> Yeah, that's why I wrote "I get the idea". :) But as I write above, I'm >> concerned about the comments getting outdated or inconsistent over time. >> >> Besides that, it more seems like something your favorite editor should help with >> instead. >> >>> Do you want me to wait for additional comments on this series, or should >>> I make the update and repost it? Thanks, >> >> As mentioned, I tend to think we should just drop them, but I'm happy to hear >> some more opinions on this if any. > > For safety I would keep something like the > > // CALC: `imem_load_size + interface_offset` > > you suggested. From simple operations yes, the code would be obvious, > but there are also more involved computations where order matters and it > is good to have a reference. These shouldn't change often anyway, and > the `CALC:` header catches the attention of anyone who would update > them, similarly to a `SAFETY:` comment. > > If Joel agrees, I will amend the comments accordingly in my staging > branch. This approach sounds good to me. I am of the opinion, this "pseudocode comment" should not change as long as the actual code's changes does not cause arithmetic changes. Whatever we decide, thanks for fixing it up Alex. -- Joel Fernandes
On Tue Jan 27, 2026 at 5:23 AM JST, Joel Fernandes wrote:
> Use checked_add() and checked_mul() when computing offsets from
> firmware-provided values in new_fwsec().
>
> Without checked arithmetic, corrupt firmware could cause integer overflow. The
> danger is not just wrapping to a huge value, but potentially wrapping to a
> small plausible offset that passes validation yet accesses entirely wrong data,
> causing silent corruption or security issues.
>
> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
> drivers/gpu/nova-core/firmware/fwsec.rs | 60 ++++++++++++++-----------
> 1 file changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
> index a8ec08a500ac..71541b1f07d7 100644
> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
> @@ -46,10 +46,7 @@
> Signed,
> Unsigned, //
> },
> - num::{
> - FromSafeCast,
> - IntoSafeCast, //
> - },
> + num::FromSafeCast,
> vbios::Vbios,
> };
>
> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
> let ucode = bios.fwsec_image().ucode(&desc)?;
> let mut dma_object = DmaObject::from_data(dev, ucode)?;
>
> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
> + // Compute hdr_offset = imem_load_size + interface_offset.
> + let hdr_offset = desc
> + .imem_load_size()
> + .checked_add(desc.interface_offset())
> + .map(usize::from_safe_cast)
> + .ok_or(EINVAL)?;
> // SAFETY: we have exclusive access to `dma_object`.
Missing empty line before the SAFETY comment (also in other places).
I will fix when applying, no need to resend.
On Wed Jan 28, 2026 at 4:58 PM JST, Alexandre Courbot wrote:
> On Tue Jan 27, 2026 at 5:23 AM JST, Joel Fernandes wrote:
>> Use checked_add() and checked_mul() when computing offsets from
>> firmware-provided values in new_fwsec().
>>
>> Without checked arithmetic, corrupt firmware could cause integer overflow. The
>> danger is not just wrapping to a huge value, but potentially wrapping to a
>> small plausible offset that passes validation yet accesses entirely wrong data,
>> causing silent corruption or security issues.
>>
>> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> ---
>> drivers/gpu/nova-core/firmware/fwsec.rs | 60 ++++++++++++++-----------
>> 1 file changed, 35 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
>> index a8ec08a500ac..71541b1f07d7 100644
>> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
>> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
>> @@ -46,10 +46,7 @@
>> Signed,
>> Unsigned, //
>> },
>> - num::{
>> - FromSafeCast,
>> - IntoSafeCast, //
>> - },
>> + num::FromSafeCast,
>> vbios::Vbios,
>> };
>>
>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>> let ucode = bios.fwsec_image().ucode(&desc)?;
>> let mut dma_object = DmaObject::from_data(dev, ucode)?;
>>
>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
>> + // Compute hdr_offset = imem_load_size + interface_offset.
>> + let hdr_offset = desc
>> + .imem_load_size()
>> + .checked_add(desc.interface_offset())
>> + .map(usize::from_safe_cast)
>> + .ok_or(EINVAL)?;
>> // SAFETY: we have exclusive access to `dma_object`.
>
> Missing empty line before the SAFETY comment (also in other places).
>
> I will fix when applying, no need to resend.
I also got this clippy warning when building:
warning: unsafe block missing a safety comment
--> ../drivers/gpu/nova-core/firmware/fwsec.rs:303:17
|
303 | unsafe { transmute_mut(&mut dma_object, dmem_mapper_offset) }?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
= note: requested on the command line with `-W clippy::undocumented-unsafe-blocks`
warning: unsafe block missing a safety comment
--> ../drivers/gpu/nova-core/firmware/fwsec.rs:319:17
|
319 | unsafe { transmute_mut(&mut dma_object, frts_cmd_offset) }?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding a safety comment on the preceding line
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
warning: 2 warnings emitted
Since the `unsafe` keyword has moved to a new line, its SAFETY comment needed
to be moved right above it, despite it still being part of the same statement.
I'll fix this as well.
On 1/28/2026 3:08 AM, Alexandre Courbot wrote:
> On Wed Jan 28, 2026 at 4:58 PM JST, Alexandre Courbot wrote:
>> On Tue Jan 27, 2026 at 5:23 AM JST, Joel Fernandes wrote:
>>> Use checked_add() and checked_mul() when computing offsets from
>>> firmware-provided values in new_fwsec().
>>>
>>> Without checked arithmetic, corrupt firmware could cause integer overflow. The
>>> danger is not just wrapping to a huge value, but potentially wrapping to a
>>> small plausible offset that passes validation yet accesses entirely wrong data,
>>> causing silent corruption or security issues.
>>>
>>> Reviewed-by: Zhi Wang <zhiw@nvidia.com>
>>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>>> ---
>>> drivers/gpu/nova-core/firmware/fwsec.rs | 60 ++++++++++++++-----------
>>> 1 file changed, 35 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
>>> index a8ec08a500ac..71541b1f07d7 100644
>>> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
>>> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
>>> @@ -46,10 +46,7 @@
>>> Signed,
>>> Unsigned, //
>>> },
>>> - num::{
>>> - FromSafeCast,
>>> - IntoSafeCast, //
>>> - },
>>> + num::FromSafeCast,
>>> vbios::Vbios,
>>> };
>>>
>>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>> let ucode = bios.fwsec_image().ucode(&desc)?;
>>> let mut dma_object = DmaObject::from_data(dev, ucode)?;
>>>
>>> - let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
>>> + // Compute hdr_offset = imem_load_size + interface_offset.
>>> + let hdr_offset = desc
>>> + .imem_load_size()
>>> + .checked_add(desc.interface_offset())
>>> + .map(usize::from_safe_cast)
>>> + .ok_or(EINVAL)?;
>>> // SAFETY: we have exclusive access to `dma_object`.
>>
>> Missing empty line before the SAFETY comment (also in other places).
>>
>> I will fix when applying, no need to resend.
>
> I also got this clippy warning when building:
>
> warning: unsafe block missing a safety comment
> --> ../drivers/gpu/nova-core/firmware/fwsec.rs:303:17
> |
> 303 | unsafe { transmute_mut(&mut dma_object, dmem_mapper_offset) }?;
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> |
> = help: consider adding a safety comment on the preceding line
> = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
> = note: requested on the command line with `-W clippy::undocumented-unsafe-blocks`
>
> warning: unsafe block missing a safety comment
> --> ../drivers/gpu/nova-core/firmware/fwsec.rs:319:17
> |
> 319 | unsafe { transmute_mut(&mut dma_object, frts_cmd_offset) }?;
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> |
> = help: consider adding a safety comment on the preceding line
> = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
>
> warning: 2 warnings emitted
>
> Since the `unsafe` keyword has moved to a new line, its SAFETY comment needed
> to be moved right above it, despite it still being part of the same statement.
> I'll fix this as well.
Thanks Alex! Do you mind also dropping those "Compute .." comments that
Danilo mentioned. But come to think of it, I think those comments do
improve any loss of readability due to the checked_* calls.
--
Joel Fernandes
© 2016 - 2026 Red Hat, Inc.