search

dgibson / dtc

Device Tree Compiler
218 stars 130 forks source link

Fix a UB when fdt_get_string return null #65

Closed yujincheng08 closed 2 years ago

yujincheng08 commented 2 years ago

When fdt_get_string return null, namep is not correctly reset.

dgibson commented 2 years ago

Sorry I've taken so long to look at this.

I don't see why this change is desirable. If we're returning an error, the namep value isn't meaningful, so I'm not sure why we need to set it. It certainly doesn't look like UB in the compiler sense to me, although a caller might trigger UB if it used the namep value without checking for errors first.

In order to apply this, I'd need at minimum, a more detailed rationale in the commit message, and also a Signed-off-by line.

yujincheng08 commented 2 years ago

@dgibson Thanks for your reply. I have updated as requested.

Basically, this fixes the consistency of namep and lenp. In the document, they will be overwritten by the function fdt_getprop_by_offset without having an exception of error occurrence. And actually, lenp is always overwritten but namep will be kept untouched inconsistently when an error occurred. The caller may trigger an ub if they assume namep is overwritten when error occurs like https://github.com/topjohnwu/Magisk/blob/e097c097feb881f6097b6d1dc346f310bc92f5d6/native/jni/magiskboot/dtb.cpp#L42.

dgibson commented 2 years ago

I'm not sure which documentation of fdt_getprop_by_offset you're referring to. The inline documentation in libfdt.h only lists namep being updated under the case where a valid property value is returned.

The example code you linked doesn't appear to check for failure at all. It also dereferences value (as value[0]) before checking if it is NULL. That will cause UB regardless of whether namep is initialized.

yujincheng08 commented 2 years ago

@dgibson I referred to this: https://github.com/dgibson/dtc/blob/45f3d1a095dd3440578d5c6313eba555a791f3fb/libfdt/libfdt.h#L727-L728

It says both namep and lenp will be overwritten (without exception about error).

And the code I show checks size first and then check value. So if *lenp is set correctly to 0 on error, the code will be safe. Sorry, I know the example codes are bad... I just want to show you the inconsistency of lenp and namep, where lenp is set to 0 on error while namep keeps untouched.

dgibson commented 2 years ago

@dgibson I referred to this:

https://github.com/dgibson/dtc/blob/45f3d1a095dd3440578d5c6313eba555a791f3fb/libfdt/libfdt.h#L727-L728

It says both namep and lenp will be overwritten (without exception about error).

Ok, I take your point. Applying.