On 09/18/2012 04:00 PM, Peter Krempa wrote:
> The DAC security driver silently ignored errors when parsing the DAC
> label and used default values instead.
>
> With a domain containing the following label definition:
>
> <seclabel type='static' model='dac' relabel='yes'>
> <label>sdfklsdjlfjklsdjkl</label>
> </seclabel>
>
> the domain would start normaly but the disk images would be still owned
> by root and no error was displayed.
>
> This patch changes the behavior if the parsing of the label fails (note
> that a not present label is not a failure and in this case the default
> label should be used) the error isn't masked but is raised that causes
> the domain start to fail with a descriptive error message:
>
> virsh # start tr
> error: Failed to start domain tr
> error: invalid argument: failed to parse uid and gid for DAC security
> driver: sdfklsdjlfjklsdjkl
>
> I also changed the error code to "invalid argument" from "internal
> error".
> ---
> Marcelo Cerri pointed out that there's another function doing the same
> that I missed in v1. This version fixes both of them.
>
> src/security/security_dac.c | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 211fb37..ea08b42 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -90,6 +90,7 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
> return 0;
> }
>
> +/* returns 1 if label isn't found, 0 on success, -1 on error */
> static
> int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
> {
> @@ -98,18 +99,18 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr)
> virSecurityLabelDefPtr seclabel;
>
> if (def == NULL)
> - return -1;
> + return 1;
>
> seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> if (seclabel == NULL || seclabel->label == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("security label for DAC not found in domain %s"),
> def->name);
Since this error gets masked, I was thinking it would would be better to
change it to VIR_ERROR or even VIR_WARN, but ...
> - return -1;
> + return 1;
> }
>
> if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> + virReportError(VIR_ERR_INVALID_ARG,
> _("failed to parse uid and gid for DAC "
> "security driver: %s"), seclabel->label);
> return -1;
> @@ -127,8 +128,10 @@ static
> int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> uid_t *uidPtr, gid_t *gidPtr)
> {
> - if (virSecurityDACParseIds(def, uidPtr, gidPtr) == 0)
> - return 0;
> + int ret;
> +
> + if ((ret = virSecurityDACParseIds(def, uidPtr, gidPtr)) <= 0)
> + return ret;
>
> if (priv) {
... in case this condition is false, the function returns -1 and there
is no error to report. That can escalate up to starting the domain which
will consequently fail without an error.
The same applies for the image label below.
> if (uidPtr)
> @@ -140,6 +143,8 @@ int virSecurityDACGetIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> return -1;
> }
>
> +
> +/* returns 1 if label isn't found, 0 on success, -1 on error */
> static
> int virSecurityDACParseImageIds(virDomainDefPtr def,
> uid_t *uidPtr, gid_t *gidPtr)
> @@ -149,19 +154,19 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
> virSecurityLabelDefPtr seclabel;
>
> if (def == NULL)
> - return -1;
> + return 1;
>
> seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
> if (seclabel == NULL || seclabel->imagelabel == NULL) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("security label for DAC not found in domain %s"),
> def->name);
> - return -1;
> + return 1;
> }
>
> if (seclabel->imagelabel
> && parseIds(seclabel->imagelabel, &uid, &gid)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> + virReportError(VIR_ERR_INVALID_ARG,
> _("failed to parse uid and gid for DAC "
> "security driver: %s"), seclabel->label);
> return -1;
> @@ -179,8 +184,10 @@ static
> int virSecurityDACGetImageIds(virDomainDefPtr def, virSecurityDACDataPtr priv,
> uid_t *uidPtr, gid_t *gidPtr)
> {
> - if (virSecurityDACParseImageIds(def, uidPtr, gidPtr) == 0)
> - return 0;
> + int ret;
> +
> + if ((ret = virSecurityDACParseImageIds(def, uidPtr, gidPtr)) <= 0)
> + return ret;
>
> if (priv) {
> if (uidPtr)
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list